[PATCH] D20645: Avoid doing binary search.

Sean Silva via llvm-commits llvm-commits at lists.llvm.org
Wed May 25 15:05:33 PDT 2016

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.


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


More information about the llvm-commits mailing list