From: Ivan Zakharyaschev <imz@altlinux.org> To: ALT Linux Team development discussions <devel@lists.altlinux.org> Cc: darktemplar@altlinux.org Subject: Re: [devel] re APT patch: Use same [32bit] type for all offsets to dynamically allocated map Date: Thu, 13 Feb 2020 08:34:49 +0300 (MSK) Message-ID: <alpine.LFD.2.20.2002130825200.6363@imap.altlinux.org> (raw) In-Reply-To: <alpine.LFD.2.20.2002130352160.6363@imap.altlinux.org> [-- Attachment #1: Type: text/plain, Size: 15084 bytes --] On Thu, 13 Feb 2020, Ivan Zakharyaschev wrote: > Со временем я замечаю больше тонкостей про memory management в APT, > например, обратил-таки когда-то внимание на следующее место в mmap.h: > > /* This should be a 32 bit type, larger tyes use too much ram and smaller > types are too small. Where ever possible 'unsigned long' should be used > instead of this internal type */ > typedef unsigned int map_ptrloc; > > Я пытался догадаться, что имели в виду авторы, и подумал, что когда > нам нужно хранить указатель в структуре, попадающей в область памяти, > отмапленную в файл ("кеш" информации о всех пакетах), то мы хотим > чтобы это всё занимало поменьше места и вместо указателя храним в > структуре "индекс" (offset) в массиве, соответствующем этой области, и > делаем его 32-битным (грубо говоря). (На LP64-платформах, таких как > x86_64, int 32битный.) > > При этом авторы призывают где возможно использовать unsigned long > (64-бита, как и указатели, на x86_64). "Где возможно" -- это, наверное, > до тех пор, пока мы индекс не записываем в структуру, которая будет > сохранена в этом мапе. Т.е. когда нам просто надо поработать с > данными. > Но всё же меня озадачивает, что следующий патч идёт как бы вразрез с этим > призывом -- он изживает unsigned long из всех методов класса DynamicMMap, > которые возвращают такие индексы. > В ветке sisyphus_one_more_time в > git://git.altlinux.org/people/darktemplar/packages/apt.git коммит > > commit 971eae5c0248f6f849e1e989cab7244e33fbaf67 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Mon Jul 22 14:18:13 2019 +0300 > > dynamic memory management: Use same type for all offsets to dynamically allocated map > > Conformance update > > Change-Id: I5efcdb9e6604ad4e3f7329e590ab0b48e5400664 Тем самым, с одной стороны, это изменение не является по виду просто эквивалентным переписыванием имевшегося кода, которое однозначно не меняет поведение. (Когда раньше смотрел этот патч, я этого не осознавал, потому что не обращал внимание, что это два типа разного размера.) С другой стороны, если поведение программы в каких-то местах может поменяться, из коммита непонятно, чем потенциальное новое поведение лучше старого. > diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc > index 5e9cfbce9..743e35723 100644 > --- a/apt/apt-pkg/contrib/mmap.cc > +++ b/apt/apt-pkg/contrib/mmap.cc > @@ -215,7 +215,7 @@ DynamicMMap::~DynamicMMap() > // DynamicMMap::RawAllocate - Allocate a raw chunk of unaligned space /*{{{*/ > // --------------------------------------------------------------------- > /* This allocates a block of memory aligned to the given size */ > -std::experimental::optional<unsigned long> DynamicMMap::RawAllocate(unsigned long long Size,unsigned long Aln) > +std::experimental::optional<map_ptrloc> DynamicMMap::RawAllocate(unsigned long long Size,unsigned long Aln) > { > unsigned long long Result = iSize; > if (Aln != 0) > @@ -231,25 +231,25 @@ std::experimental::optional<unsigned long> DynamicMMap::RawAllocate(unsigned lon > "Current values are: %llu, %llu. (man 5 apt.conf)"), > (unsigned long long) _config->FindI("APT::Cache-Start", 24*1024*1024), > (unsigned long long) _config->FindI("APT::Cache-Limit", 0)); > - return std::experimental::optional<unsigned long>(); > + return std::experimental::optional<map_ptrloc>(); > } > } > > iSize = Result + Size; > > - return std::experimental::optional<unsigned long>(Result); > + return std::experimental::optional<map_ptrloc>(Result); > } > /*}}}*/ > // DynamicMMap::Allocate - Pooled aligned allocation /*{{{*/ > // --------------------------------------------------------------------- > /* This allocates an Item of size ItemSize so that it is aligned to its > size in the file. */ > -std::experimental::optional<unsigned long> DynamicMMap::Allocate(unsigned long ItemSize) > +std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long ItemSize) > { > if (ItemSize == 0) > { > _error->Error("Can't allocate an item of size zero"); > - return std::experimental::optional<unsigned long>(); > + return std::experimental::optional<map_ptrloc>(); > } > > // Look for a matching pool entry > @@ -286,7 +286,7 @@ std::experimental::optional<unsigned long> DynamicMMap::Allocate(unsigned long I > if (Empty == 0) > { > _error->Error("Ran out of allocation pools"); > - return std::experimental::optional<unsigned long>(); > + return std::experimental::optional<map_ptrloc>(); > } > > I = Empty; > @@ -316,13 +316,13 @@ std::experimental::optional<unsigned long> DynamicMMap::Allocate(unsigned long I > > I->Count--; > I->Start += ItemSize; > - return std::experimental::optional<unsigned long>(Result/ItemSize); > + return std::experimental::optional<map_ptrloc>(Result/ItemSize); > } > /*}}}*/ > // DynamicMMap::WriteString - Write a string to the file /*{{{*/ > // --------------------------------------------------------------------- > /* Strings are not aligned to anything */ > -std::experimental::optional<unsigned long> DynamicMMap::WriteString(const char *String, > +std::experimental::optional<map_ptrloc> DynamicMMap::WriteString(const char *String, > unsigned long Len) > { > if (Len == std::numeric_limits<unsigned long>::max()) > @@ -331,7 +331,7 @@ std::experimental::optional<unsigned long> DynamicMMap::WriteString(const char * > auto Result = RawAllocate(Len+1,0); > > if (Base == NULL || !Result) > - return std::experimental::optional<unsigned long>(); > + return std::experimental::optional<map_ptrloc>(); > > memcpy((char *)Base + *Result,String,Len); > ((char *)Base)[*Result + Len] = 0; > diff --git a/apt/apt-pkg/contrib/mmap.h b/apt/apt-pkg/contrib/mmap.h > index cfeec12b1..e4f289a5c 100644 > --- a/apt/apt-pkg/contrib/mmap.h > +++ b/apt/apt-pkg/contrib/mmap.h > @@ -101,10 +101,10 @@ class DynamicMMap : public MMap > public: > > // Allocation > - std::experimental::optional<unsigned long> RawAllocate(unsigned long long Size,unsigned long Aln = 0); > - std::experimental::optional<unsigned long> Allocate(unsigned long ItemSize); > - std::experimental::optional<unsigned long> WriteString(const char *String,unsigned long Len = std::numeric_limits<unsigned long>::max()); > - inline std::experimental::optional<unsigned long> WriteString(const string &S) {return WriteString(S.c_str(),S.length());}; > + std::experimental::optional<map_ptrloc> RawAllocate(unsigned long long Size,unsigned long Aln = 0); > + std::experimental::optional<map_ptrloc> Allocate(unsigned long ItemSize); > + std::experimental::optional<map_ptrloc> WriteString(const char *String,unsigned long Len = std::numeric_limits<unsigned long>::max()); > + inline std::experimental::optional<map_ptrloc> WriteString(const string &S) {return WriteString(S.c_str(),S.length());}; > void UsePools(Pool &P,unsigned int Count) {Pools = &P; PoolCount = Count;}; > > DynamicMMap(FileFd &F,unsigned long Flags,unsigned long long WorkSpace = 2*1024*1024, > diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc > index 36d54504e..4940ec6e5 100644 > --- a/apt/apt-pkg/pkgcachegen.cc > +++ b/apt/apt-pkg/pkgcachegen.cc > @@ -525,15 +525,15 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, > // CacheGenerator::NewVersion - Create a new Version /*{{{*/ > // --------------------------------------------------------------------- > /* This puts a version structure in the linked list */ > -std::experimental::optional<unsigned long> pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, > +std::experimental::optional<map_ptrloc> pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, > const string &VerStr, > - unsigned long Next) > + map_ptrloc Next) > { > // Get a structure > const auto Version = AllocateInMap(sizeof(pkgCache::Version)); > const auto idxVerStr = WriteStringInMap(VerStr); > if ((!Version) || (!idxVerStr)) > - return std::experimental::optional<unsigned long>(); > + return std::experimental::optional<map_ptrloc>(); > > // Fill it in > Ver = pkgCache::VerIterator(Cache,Cache.VerP + *Version); > @@ -710,7 +710,7 @@ bool pkgCacheGenerator::SelectFile(const string &File, const string &Site, > // --------------------------------------------------------------------- > /* This is used to create handles to strings. Given the same text it > always returns the same number */ > -std::experimental::optional<unsigned long> pkgCacheGenerator::WriteUniqString(const char *S, > +std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const char *S, > unsigned int Size) > { > /* We use a very small transient hash table here, this speeds up generation > @@ -718,7 +718,7 @@ std::experimental::optional<unsigned long> pkgCacheGenerator::WriteUniqString(co > pkgCache::StringItem *&Bucket = UniqHash[(S[0]*5 + S[1]) % _count(UniqHash)]; > if (Bucket != 0 && > stringcmp(S,S+Size,Cache.StrP + Bucket->String) == 0) > - return std::experimental::optional<unsigned long>(Bucket->String); > + return std::experimental::optional<map_ptrloc>(Bucket->String); > > // Search for an insertion point > pkgCache::StringItem *I = Cache.StringItemP + Cache.HeaderP->StringList; > @@ -736,7 +736,7 @@ std::experimental::optional<unsigned long> pkgCacheGenerator::WriteUniqString(co > if (Res == 0) > { > Bucket = I; > - return std::experimental::optional<unsigned long>(I->String); > + return std::experimental::optional<map_ptrloc>(I->String); > } > > // Get a structure > @@ -744,7 +744,7 @@ std::experimental::optional<unsigned long> pkgCacheGenerator::WriteUniqString(co > const auto Item = AllocateInMap(sizeof(pkgCache::StringItem)); > const auto idxString = WriteStringInMap(S, Size); > if ((!Item) || (!idxString)) > - return std::experimental::optional<unsigned long>(); > + return std::experimental::optional<map_ptrloc>(); > > if (oldMap != Map.Data()) > { > @@ -760,7 +760,7 @@ std::experimental::optional<unsigned long> pkgCacheGenerator::WriteUniqString(co > ItemP->String = *idxString; > > Bucket = ItemP; > - return std::experimental::optional<unsigned long>(ItemP->String); > + return std::experimental::optional<map_ptrloc>(ItemP->String); > } > /*}}}*/ > > diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h > index dfcadbf8e..8a66765cc 100644 > --- a/apt/apt-pkg/pkgcachegen.h > +++ b/apt/apt-pkg/pkgcachegen.h > @@ -116,15 +116,15 @@ class pkgCacheGenerator > bool FoundFileDeps; > > bool NewFileVer(pkgCache::VerIterator &Ver,ListParser &List); > - std::experimental::optional<unsigned long> NewVersion(pkgCache::VerIterator &Ver,const string &VerStr,unsigned long Next); > + std::experimental::optional<map_ptrloc> NewVersion(pkgCache::VerIterator &Ver,const string &VerStr, map_ptrloc Next); > > public: > > // CNC:2003-02-27 - We need this in rpmListParser. > bool NewPackage(pkgCache::PkgIterator &PkgI,const string &Pkg); > > - std::experimental::optional<unsigned long> WriteUniqString(const char *S,unsigned int Size); > - inline std::experimental::optional<unsigned long> WriteUniqString(const string &S) {return WriteUniqString(S.c_str(),S.length());}; > + std::experimental::optional<map_ptrloc> WriteUniqString(const char *S,unsigned int Size); > + inline std::experimental::optional<map_ptrloc> WriteUniqString(const string &S) {return WriteUniqString(S.c_str(),S.length());}; > > void DropProgress() {Progress = 0;}; > bool SelectFile(const string &File,const string &Site,pkgIndexFile const &Index, > @@ -162,10 +162,10 @@ class pkgCacheGenerator::ListParser > pkgCacheGenerator *Owner; > friend class pkgCacheGenerator; > > - inline std::experimental::optional<unsigned long> WriteUniqString(const string &S) {return Owner->WriteUniqString(S);}; > - inline std::experimental::optional<unsigned long> WriteUniqString(const char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);}; > - inline std::experimental::optional<unsigned long> WriteString(const string &S) {return Owner->WriteStringInMap(S);}; > - inline std::experimental::optional<unsigned long> WriteString(const char *S,unsigned int Size) {return Owner->WriteStringInMap(S,Size);}; > + inline std::experimental::optional<map_ptrloc> WriteUniqString(const string &S) {return Owner->WriteUniqString(S);}; > + inline std::experimental::optional<map_ptrloc> WriteUniqString(const char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);}; > + inline std::experimental::optional<map_ptrloc> WriteString(const string &S) {return Owner->WriteStringInMap(S);}; > + inline std::experimental::optional<map_ptrloc> WriteString(const char *S,unsigned int Size) {return Owner->WriteStringInMap(S,Size);}; > bool NewDepends(pkgCache::VerIterator &Ver, const string &Package, > const string &Version,unsigned int Op, > unsigned int Type); > diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc > index 0cbb5f750..ea27d11c5 100644 > --- a/apt/apt-pkg/rpm/rpmlistparser.cc > +++ b/apt/apt-pkg/rpm/rpmlistparser.cc > @@ -75,7 +75,7 @@ rpmListParser::~rpmListParser() > // ListParser::UniqFindTagWrite - Find the tag and write a unq string /*{{{*/ > // --------------------------------------------------------------------- > /* */ > -std::experimental::optional<unsigned long> rpmListParser::UniqFindTagWrite(int Tag) > +std::experimental::optional<map_ptrloc> rpmListParser::UniqFindTagWrite(int Tag) > { > char *Start; > char *Stop; > @@ -90,7 +90,7 @@ std::experimental::optional<unsigned long> rpmListParser::UniqFindTagWrite(int T > * But since cacheiterators treat zero as special value, > * just pass it instead of failing > */ > - return std::experimental::optional<unsigned long>(0); > + return std::experimental::optional<map_ptrloc>(0); > } > > if (type == RPM_STRING_TYPE) > diff --git a/apt/apt-pkg/rpm/rpmlistparser.h b/apt/apt-pkg/rpm/rpmlistparser.h > index 6767d106d..22a90a0a5 100644 > --- a/apt/apt-pkg/rpm/rpmlistparser.h > +++ b/apt/apt-pkg/rpm/rpmlistparser.h > @@ -45,7 +45,7 @@ class rpmListParser : public pkgCacheGenerator::ListParser > > bool Duplicated; > > - std::experimental::optional<unsigned long> UniqFindTagWrite(int Tag); > + std::experimental::optional<map_ptrloc> UniqFindTagWrite(int Tag); > bool ParseStatus(pkgCache::PkgIterator &Pkg,pkgCache::VerIterator &Ver); > bool ParseDepends(pkgCache::VerIterator &Ver, > char **namel, char **verl, int32_t *flagl, > > -- > Best regards, > Ivan
next prev parent reply other threads:[~2020-02-13 5:34 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-02-13 1:13 Ivan Zakharyaschev 2020-02-13 2:19 ` Vladislav Zavjalov 2020-02-13 5:34 ` Ivan Zakharyaschev [this message] 2020-02-13 12:33 ` Aleksei Nikiforov 2020-02-13 14:05 ` Ivan Zakharyaschev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.LFD.2.20.2002130825200.6363@imap.altlinux.org \ --to=imz@altlinux.org \ --cc=darktemplar@altlinux.org \ --cc=devel@lists.altlinux.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
ALT Linux Team development discussions This inbox may be cloned and mirrored by anyone: git clone --mirror http://lore.altlinux.org/devel/0 devel/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 devel devel/ http://lore.altlinux.org/devel \ devel@altlinux.org devel@altlinux.ru devel@lists.altlinux.org devel@lists.altlinux.ru devel@linux.iplabs.ru mandrake-russian@linuxteam.iplabs.ru sisyphus@linuxteam.iplabs.ru public-inbox-index devel Example config snippet for mirrors. Newsgroup available over NNTP: nntp://lore.altlinux.org/org.altlinux.lists.devel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git