[PATCH] D46145: Use a buffer when allocating relocations

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Writer.cpp:875
+  };
+
   // Scan all relocations. Each relocation goes through a series
----------------
espindola wrote:
> grimar wrote:
> > espindola wrote:
> > > grimar wrote:
> > > > It can be a bit simpler I think:
> > > > (RAM profiling shows that the version below consume the same amount of memory).
> > > > 
> > > > ```
> > > > std::vector<Relocation> Buffer;
> > > > 
> > > > // <new comment>
> > > > auto WithBuffer = [&](InputSectionBase &Sec) {
> > > >   Fn(Sec);
> > > >   Buffer = std::move(Sec.Relocations);
> > > >   Sec.Relocations = Buffer;
> > > > };
> > > > ```
> > > That would reduce peak allocation, but each call to Fn is still allocating a new buffer, no?
> > > 
> > > But does suggest a way to simplify it a bit.
> > CPU time was about the same as the original code for my way I think.
> > Your way should be slightly faster though:
> > 
> > I observed a minor speed up between original and your version. It was:
> > Original CPU time total (3 runs): 2632, 2637, 2622.
> > Patch: 2604, 2610, 2592.
> > 
> > So it is about 0.5% I think. Results seem was more or less stable. I do not have precise numbers for my version underhand (I can retest it if you think it worth, but it was something about original code numbers). I think the simplicity of implementation makes more sense here probably (and I hope we will be able to use `shrink_to_fit` one day). 
> Testing the suggestion the peak memory is indeed the same, but the total memory allocated is actually higher:
> 
> master 765.92  564.2
> patch  748.16, 545.7
> patch2 865.48, 545.7
Did not see that (my profiler seem shows only peak memory).
Then, do we really need to care too much about total memory allocated if performance and peak memory consumption are about the same?


https://reviews.llvm.org/D46145