[devel] re APT patch: Use same [32bit] type for all offsets to dynamically allocated map
Ivan Zakharyaschev
imz на altlinux.org
Чт Фев 13 17:05:56 MSK 2020
On Thu, 13 Feb 2020, Aleksei Nikiforov wrote:
> 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 <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
> >
> > Тем самым, с одной стороны, это изменение не является по виду просто
> > эквивалентным переписыванием имевшегося кода, которое однозначно не меняет
> > поведение. (Когда раньше смотрел этот патч, я этого не осознавал, потому
> > что не обращал внимание, что это два типа разного размера.)
> >
> > С другой стороны, если поведение программы в каких-то местах может
> > поменяться, из коммита непонятно, чем потенциальное новое поведение лучше
> > старого.
> Далее по поводу данного патча и некоторых предыдущих патчей из серии.
>
> Портируемые из Debian изменения, как и сделанные на их основе мной изменения,
> содержали unsigned long как до изменений, так и после. Поэтому я решил это не
> менять там.
Согласен, что так лучше, когда это неочевидное по последствиям изменение
отделено от других в отдельном коммите.
> Поскольку индексы map_ptrloc и 'unsigned long' указывают на
> смещение в одном и том же файле, делать их разными типами смысла никакого нет.
Это не ясно до конца, потому что до этого изменения часть работы с ними
делалась в виде unsigned long. Только часть этих индексов могла доехать до
того места, где мы сохраняем их в мап в виде 32-битного числа.
Вот эта часть работы может измениться. (А может быть и нет.) Это неясно из
commit message. Оно немного вводит в заблуждение, называя это conformance
change, как будто бы нет потенциального изменения поведения, которое
требует обоснования. (Чем новое поведение, именно в run-time, а не по
warning-ам от компилятора, лучше старого?)
> Соответственно, я привёл в этом патче их к единому виду: map_ptrloc. Сейчас
> это typedef на 'unsigned int'. Делать map_ptrloc более длинным типом я причин
> пока что не вижу. Как минимум, пока файлы кэша не подходят близко к отметке в
> 4Гб. А вот минусы от такого изменения быть могут, и скорее всего будут, в виде
> увеличения потребления ресурсов.
Согласен, что хранить в мапе в 64-битном виде не надо, раз пока и так
справлялись. Менять ещё и эту часть я не предлагал и не хотел.
> Только что посмотрел код 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<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
> > >
> > > _______________________________________________
> > > Devel mailing list
> > > Devel на lists.altlinux.org
> > > https://lists.altlinux.org/mailman/listinfo/devel
> _______________________________________________
> Devel mailing list
> Devel на lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
Подробная информация о списке рассылки Devel