[PATCH] D20645: Avoid doing binary search.

[Sean Silva via llvm-commits]

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

These are the timings on windows: http://reviews.llvm.org/F1987812
F1987812: Screen Shot 2016-05-26 at 9.00.49 PM.png <http://reviews.llvm.org/F1987812>

Looks like this patch (labeled "rui") gives some speedup. I'm still a bit worried about the extra memory usage (on principle of not being a memory hog), but since this doesn't seem to pessimize anything then this seems fine.

A couple nits inline, but this LGTM.


================
Comment at: ELF/InputSection.cpp:539
@@ +538,3 @@
+  // In that case we need to search from the original section piece vector.
+  // It is a slow path because binary search is slower than hash table lookup
+  // due to CPU's branch misprediction.
----------------
These branches may be less predictable than typical branches, but I think it is premature (based on what I've seen presented as analysis) to specifically say that the slowdown is due to branch misprediction.

For example, the fact that changing the sizeof in r270717 made a significant performance difference suggests that there is a strong influence of memory access too and that branch misprediction isn't the only factor.

(see e.g. the things I linked earlier; the performance of binary search is actually quite complex and, broadly speaking, the performance difference between binary search and hash tables is due to memory, not branch prediction).

So I would prefer to remove this comment.

================
Comment at: ELF/InputSection.cpp:548
@@ +547,3 @@
+  if (!Config->Threads)
+    OffsetMap[Offset] = Ret;
+  return Ret;
----------------
I'd really like to avoid this `if (!Config->Threads)` thing. For example, this is a confounding factor if somebody wants to analyze LLD's scalability with threads, so I'd rather we not have it unless it makes a huge difference (which IIRC you said it didn't).


http://reviews.llvm.org/D20645