From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=unavailable autolearn_force=no version=3.4.1 To: Ivan Zakharyaschev References: From: Aleksei Nikiforov Message-ID: <508afae9-86ab-52ac-6580-40cd79c54997@altlinux.org> Date: Thu, 13 Feb 2020 15:33:00 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=koi8-r; format=flowed Content-Language: ru Content-Transfer-Encoding: 8bit Cc: ALT Linux Team development discussions Subject: Re: [devel] re APT patch: Use same [32bit] type for all offsets to dynamically allocated map X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Feb 2020 12:33:07 -0000 Archived-At: List-Archive: List-Post: 13.02.2020 08:34, Ivan Zakharyaschev пишет: > > 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 >> 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 > > Тем самым, с одной стороны, это изменение не является по виду просто > эквивалентным переписыванием имевшегося кода, которое однозначно не меняет > поведение. (Когда раньше смотрел этот патч, я этого не осознавал, потому > что не обращал внимание, что это два типа разного размера.) > > С другой стороны, если поведение программы в каких-то местах может > поменяться, из коммита непонятно, чем потенциальное новое поведение лучше > старого. > Здравствуй. По поводу мыслей и замыслов других авторов, в том числе из Debian, я ничего сказать не могу. Такие вопросы точно не по адресу. Далее по поводу данного патча и некоторых предыдущих патчей из серии. Портируемые из Debian изменения, как и сделанные на их основе мной изменения, содержали unsigned long как до изменений, так и после. Поэтому я решил это не менять там. Поскольку индексы map_ptrloc и 'unsigned long' указывают на смещение в одном и том же файле, делать их разными типами смысла никакого нет. Соответственно, я привёл в этом патче их к единому виду: map_ptrloc. Сейчас это typedef на 'unsigned int'. Делать map_ptrloc более длинным типом я причин пока что не вижу. Как минимум, пока файлы кэша не подходят близко к отметке в 4Гб. А вот минусы от такого изменения быть могут, и скорее всего будут, в виде увеличения потребления ресурсов. Только что посмотрел код apt свежей версии из Debian. Там map_ptrloc заменили на map_stringitem_t, а unsigned long в данных случаях - на map_pointer_t, который является typedef на тот же самый map_stringitem_t, который в свою очередь всё ещё является typedef на 32-битный int, который правда прописали как uint32_t. >> 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 DynamicMMap::RawAllocate(unsigned long long Size,unsigned long Aln) >> +std::experimental::optional DynamicMMap::RawAllocate(unsigned long long Size,unsigned long Aln) >> { >> unsigned long long Result = iSize; >> if (Aln != 0) >> @@ -231,25 +231,25 @@ std::experimental::optional 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(); >> + return std::experimental::optional(); >> } >> } >> >> iSize = Result + Size; >> >> - return std::experimental::optional(Result); >> + return std::experimental::optional(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 DynamicMMap::Allocate(unsigned long ItemSize) >> +std::experimental::optional DynamicMMap::Allocate(unsigned long ItemSize) >> { >> if (ItemSize == 0) >> { >> _error->Error("Can't allocate an item of size zero"); >> - return std::experimental::optional(); >> + return std::experimental::optional(); >> } >> >> // Look for a matching pool entry >> @@ -286,7 +286,7 @@ std::experimental::optional DynamicMMap::Allocate(unsigned long I >> if (Empty == 0) >> { >> _error->Error("Ran out of allocation pools"); >> - return std::experimental::optional(); >> + return std::experimental::optional(); >> } >> >> I = Empty; >> @@ -316,13 +316,13 @@ std::experimental::optional DynamicMMap::Allocate(unsigned long I >> >> I->Count--; >> I->Start += ItemSize; >> - return std::experimental::optional(Result/ItemSize); >> + return std::experimental::optional(Result/ItemSize); >> } >> /*}}}*/ >> // DynamicMMap::WriteString - Write a string to the file /*{{{*/ >> // --------------------------------------------------------------------- >> /* Strings are not aligned to anything */ >> -std::experimental::optional DynamicMMap::WriteString(const char *String, >> +std::experimental::optional DynamicMMap::WriteString(const char *String, >> unsigned long Len) >> { >> if (Len == std::numeric_limits::max()) >> @@ -331,7 +331,7 @@ std::experimental::optional DynamicMMap::WriteString(const char * >> auto Result = RawAllocate(Len+1,0); >> >> if (Base == NULL || !Result) >> - return std::experimental::optional(); >> + return std::experimental::optional(); >> >> 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 RawAllocate(unsigned long long Size,unsigned long Aln = 0); >> - std::experimental::optional Allocate(unsigned long ItemSize); >> - std::experimental::optional WriteString(const char *String,unsigned long Len = std::numeric_limits::max()); >> - inline std::experimental::optional WriteString(const string &S) {return WriteString(S.c_str(),S.length());}; >> + std::experimental::optional RawAllocate(unsigned long long Size,unsigned long Aln = 0); >> + std::experimental::optional Allocate(unsigned long ItemSize); >> + std::experimental::optional WriteString(const char *String,unsigned long Len = std::numeric_limits::max()); >> + inline std::experimental::optional 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 pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, >> +std::experimental::optional 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(); >> + return std::experimental::optional(); >> >> // 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 pkgCacheGenerator::WriteUniqString(const char *S, >> +std::experimental::optional 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 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(Bucket->String); >> + return std::experimental::optional(Bucket->String); >> >> // Search for an insertion point >> pkgCache::StringItem *I = Cache.StringItemP + Cache.HeaderP->StringList; >> @@ -736,7 +736,7 @@ std::experimental::optional pkgCacheGenerator::WriteUniqString(co >> if (Res == 0) >> { >> Bucket = I; >> - return std::experimental::optional(I->String); >> + return std::experimental::optional(I->String); >> } >> >> // Get a structure >> @@ -744,7 +744,7 @@ std::experimental::optional pkgCacheGenerator::WriteUniqString(co >> const auto Item = AllocateInMap(sizeof(pkgCache::StringItem)); >> const auto idxString = WriteStringInMap(S, Size); >> if ((!Item) || (!idxString)) >> - return std::experimental::optional(); >> + return std::experimental::optional(); >> >> if (oldMap != Map.Data()) >> { >> @@ -760,7 +760,7 @@ std::experimental::optional pkgCacheGenerator::WriteUniqString(co >> ItemP->String = *idxString; >> >> Bucket = ItemP; >> - return std::experimental::optional(ItemP->String); >> + return std::experimental::optional(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 NewVersion(pkgCache::VerIterator &Ver,const string &VerStr,unsigned long Next); >> + std::experimental::optional 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 WriteUniqString(const char *S,unsigned int Size); >> - inline std::experimental::optional WriteUniqString(const string &S) {return WriteUniqString(S.c_str(),S.length());}; >> + std::experimental::optional WriteUniqString(const char *S,unsigned int Size); >> + inline std::experimental::optional 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 WriteUniqString(const string &S) {return Owner->WriteUniqString(S);}; >> - inline std::experimental::optional WriteUniqString(const char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);}; >> - inline std::experimental::optional WriteString(const string &S) {return Owner->WriteStringInMap(S);}; >> - inline std::experimental::optional WriteString(const char *S,unsigned int Size) {return Owner->WriteStringInMap(S,Size);}; >> + inline std::experimental::optional WriteUniqString(const string &S) {return Owner->WriteUniqString(S);}; >> + inline std::experimental::optional WriteUniqString(const char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);}; >> + inline std::experimental::optional WriteString(const string &S) {return Owner->WriteStringInMap(S);}; >> + inline std::experimental::optional 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 rpmListParser::UniqFindTagWrite(int Tag) >> +std::experimental::optional rpmListParser::UniqFindTagWrite(int Tag) >> { >> char *Start; >> char *Stop; >> @@ -90,7 +90,7 @@ std::experimental::optional rpmListParser::UniqFindTagWrite(int T >> * But since cacheiterators treat zero as special value, >> * just pass it instead of failing >> */ >> - return std::experimental::optional(0); >> + return std::experimental::optional(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 UniqFindTagWrite(int Tag); >> + std::experimental::optional 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 >> >> _______________________________________________ >> Devel mailing list >> Devel@lists.altlinux.org >> https://lists.altlinux.org/mailman/listinfo/devel