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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:1712-1715
+  auto E = llvm::remove_if(V, [](InputSectionBase *IS) {
+    return !IS || IS == &InputSection::Discarded || !IS->getSize();
+  });
+  V.erase(E, V.end());
----------------
dblaikie wrote:
> This is often done nested like this, if you like
> 
>   V.erase(llvm::remove_if(V, []...), V.end());
Yeah. I did that initially and rewrote later, because that looks a bit bulky for my eye after clang-format.
Made as nested again.


================
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:
> 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.


================
Comment at: ELF/SyntheticSections.cpp:1726
+                                });
+      if (I == V.end())
+        continue;
----------------
dblaikie wrote:
> need any error handling for if R.first isn't in any entry in V?
I though about that too. As far I understand that can happen only if we have some broken range in DWARF,
like corrupted entry in .debug_ranges section. I probably do not see other way to achieve that.
And that is a dillemma about what to do then. We can do additional check of course, but at the same time,
AFAIK LLD policy is that we are OK to produce broken output if input was broken.

I can do additional check here and error out, just don't sure if we really need it (how much the case is real to happen).


https://reviews.llvm.org/D32853