[PATCH] D32853: [ELF] - Speedup readAddressArea() implementation.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:1722
+    for (std::pair<uint64_t, uint64_t> &R : Ranges) {
+      auto I = std::lower_bound(V.begin(), V.end(), R.first,
+                                [](InputSectionBase *IS, uint64_t Offset) {
----------------
dblaikie wrote:
> grimar wrote:
> > dblaikie wrote:
> > > Is 'V' guaranteed to be sorted?
> > We initialize File->Sections in the same order they are in objects section table:
> > https://github.com/llvm-mirror/lld/blob/master/ELF/InputFiles.cpp#L282
> > 
> > From my observations input sections in objects are always being sorted by file offset.
> > (just because usual code seems assigns file offsets for sections first and then writes them in the same order or something like that)
> > That probably not a mandatory requirement, but looks to be reasonable assumption.
> > 
> > I can probably add some additional assert-check to verify array is sorted or call sort explicitly, not sure if latter worth to do.
> > 
> > Not relative to this patch:
> > Another Idea I had in mind is to make DWARFAddressRangesVector to store not only Low/High index, but also a object::SectionRef for example.
> > That would remove the need of returning fake section address from llvm::LoadedObjectInfo::getSectionLoadAddress and should allow LLD to get
> > section index from SectionRef somehow. Then we will not need any loops for doing a range section search and approach would be cleaner and faster.
> > But that would require DWARF parsers API change, I would probably not do that change right now, because it is not yet clear if we want it or not, because for example using relocated output for building index solves this problem in a different way, but at the same time I guess we can build index using multithreading and it is probably easier to not use relocated output for that and just parse each file in a parrallel loop. I am going to investigate that all and for now would go with current patch as it is simple LLD side solution that gives solid instant speedup.
> If it's not guaranteed by ELF to be sorted - best not to rely on it. You might be able to construct a test case by modifying LLVM's MC layer and having it produce the headers in reverse order (or just swapping the first two, etc).
I see, thanks for suggestion and your time. I think this patch becomes a bit complicated solution then. If we need to add sorting that probably will bring some slowdown and code complication, new testcase with swapped sections will be just temporarily needed, because if we will change the DWARF parsers API to return section index for ranges somehow instead, for example, we will not need to do any slow things like sorting+searching on LLD side anymore at all.

Today I posted patch for building .gdb_index in a multithreaded way (D33122). That patch also suffers from weakness I am trying to fix here.
If will be desided that we are fine to go with D33122, then it seems to me that it will worth to extend DWARF API instead of continue work on this patch.


https://reviews.llvm.org/D32853