[PATCH] D23836: [ThinLTO/gold] Add caching support to gold-plugin

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.

================
Comment at: llvm/trunk/tools/gold/gold-plugin.cpp:812
@@ -801,1 +811,3 @@
+          *LTOOutput(OutputName).getStream() << Buffer->getBuffer();
+        });
   };
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > tejohnson wrote:
> > > mehdi_amini wrote:
> > > > mehdi_amini wrote:
> > > > > I just noticed: this is largely suboptimal, the Buffer will be a mmap'd file in memory, and you're duplicating it here.
> > > > I didn't notice earlier because the libLTO interface returns a pointer to the memory to the linker, so the MemoryBuffer is untouched till the linker needs it (it does not need to be paged in memory, which can contributes to explains why you have much more memory pressure)
> > > > I just noticed: this is largely suboptimal, the Buffer will be a mmap'd file in memory, and you're duplicating it here.
> > > 
> > > Dumb question - where is the duplication happening? getBuffer will give back a StringRef to the memory pointed to by the MemoryBuffer. And we then stream it to an output file (which we would have done earlier without caching).
> > The StringRef points to the memory mapped from the cache. So this is data on disk that you load in memory while you access it here.
> > The duplication is that you're creating a new file, while there is already a file on disk, this wouldn't happen in the libLTO implementation.
> > 
> Ah ok, yes the file is duplicated unfortunately (temporarily), gold cleans up the temp object files written here. I didn't noticed a memory increase from this change.
> 
> Not sure if there is a way for gold-plugin to pass back the native object in memory, but I don't see a hook for that unfortunately.
Even if the peak memory does not increase, the memory traffic (and cache trashing) is probably important (let along that these data need to be flushed to disk...)


Repository:
  rL LLVM

https://reviews.llvm.org/D23836