[PATCH] D20645: Avoid doing binary search.

[Sean Silva via llvm-commits]

silvas requested changes to this revision.
silvas added a comment.
This revision now requires changes to proceed.

IMO this is not worth doing. It's only 5% when this function and surrounding code is like 25%, so this doesn't really speed things up much at all.

Also, just bailing out on keeping this performance benefit in multithreaded mode seems like a poor engineering decision.


================
Comment at: ELF/InputSection.cpp:533
@@ -528,1 +532,3 @@
 typename ELFT::uint MergeInputSection<ELFT>::getOffset(uintX_t Offset) {
+  auto It = OffsetMap.find(Offset);
+  if (It != OffsetMap.end())
----------------
Have you analyzed the pattern of this `Offset` value? I suspect there is some sort of pattern that is easier to cache than using a whole map (e.g. the values might be sequential).

(as a first-order analysis, just add a printf here and see how well gzip compresses it)

================
Comment at: ELF/InputSection.cpp:540
@@ -529,1 +539,3 @@
+  // It is a slow path because binary search is slower than hash table lookup
+  // due to CPU's branch misprediction.
   SectionPiece &Piece = *this->getSectionPiece(Offset);
----------------
Did you actually measure this? The actual tradeoffs for binary search vs a hash table are fairly complex and mostly about reducing serial dependencies through memory.

http://www.pvk.ca/Blog/2012/07/03/binary-search-star-eliminates-star-branch-mispredictions/
http://www.pvk.ca/Blog/2012/07/30/binary-search-is-a-pathological-case-for-caches/
http://arxiv.org/abs/1509.05053

================
Comment at: ELF/InputSection.cpp:547
@@ +546,3 @@
+  // Cache the result if it's safe to do so.
+  if (!Config->Threads)
+    OffsetMap[Offset] = Ret;
----------------
This is sort of counterproductive...


http://reviews.llvm.org/D20645