From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 13 Feb 2020 04:13:17 +0300 (MSK) From: Ivan Zakharyaschev To: darktemplar@altlinux.org, devel@lists.altlinux.org Message-ID: User-Agent: Alpine 2.20 (LFD 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="1807885841-849193023-1581556397=:6363" Subject: [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 01:13:18 -0000 Archived-At: List-Archive: List-Post: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --1807885841-849193023-1581556397=:6363 Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 8BIT Со временем я замечаю больше тонкостей про 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, которые возвращают такие индексы. Хотелось бы понять: авторы APT хотели бессмысленного или у их задумки был какой-то смысл и этот патч её нарушает? Связанный вопрос, о котором я задумался: в тех местах, где эти "индексы" (offsets) вычисляются (чтобы их вернуть), какие гарантии, что они не выйдут за пределы 32 бит? Возможно, с такими гарантиями лучше в тех местах в коде, где эти значения окончательно попадают в поля структур типа map_ptrloc, которые будут хранится в мапе ("кеше")? Что всё же лучше (и почему): оставить, как было, или перейти на 32 бита в соответствии с этим патчем? Если перейти, мне кажется, их комментарий с призывом к unsigned long хорошо бы тоже заменить на обоснование новой концепции (а то он будет противоречить тому, что мы видим в коде, и сбивать с толку ещё сильнее, хотя и раньше он был загадочным). В ветке 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 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 --1807885841-849193023-1581556397=:6363--