[PATCH] D116367: [ELF][LTO] Call madvise(MADV_DONTNEED) on BitcodeFile buffers

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM with a couple of comment suggestions below.

In D116367#3213058 <https://reviews.llvm.org/D116367#3213058>, @MaskRay wrote:

> In D116367#3212997 <https://reviews.llvm.org/D116367#3212997>, @tejohnson wrote:
>
>> I tried a few times and didn't see a slowdown.
>>
>> However, with the latest version to only free the bitcode files, I am seeing less memory reduction (9.3G vs 9.1G before), and also a 30-40s slowdown (maybe the DenseSet lookups?). Something could probably be done to avoid the lookups, but do you have any idea why the memory reduction would be less? Hmm, probably there are some other non-bitcode files that are being read for symbol resolution but not being used further in index only mode?
>
> It was my negligence. `--start-lib` bitcode files may not be extracted. They reside in the `lazyBitcodeFiles` list. The latest diff marked them as well.
> I think this is sufficient for our use case. The diff tried a bit more to support other use cases.

Confirmed that the memory savings recovered, and the time reduced too, but in my case skipLinkedOutput is true so it is not doing the dense set lookups at all.



================
Comment at: lld/ELF/Driver.cpp:2396
+  // not object files. Index file creation is already done in
+  // compileBitcodeFiles, so we are done if that's the case.  Likewise,
+  // --plugin-opt=emit-llvm and --plugin-opt=emit-asm are the options to create
----------------
Suggest making this something like "Index file creation is performed in compileBitcodeFiles, so we are done afterwards in that case." Or move this sentence below where we take the early return.


================
Comment at: lld/ELF/Driver.cpp:2399
+  // output files in bitcode or assembly code respectively. No object files are
+  // generated. When only certain thinLTO modules are specified for compilation.
+  // The intermediate object file are the expected output.
----------------
"When only certain thinLTO modules are specified for compilation." isn't a complete sentence. Maybe, "This is also true when only ..." ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116367/new/

https://reviews.llvm.org/D116367