[PATCH] D54418: [llvm-exegesis] InstructionBenchmarkClustering::dbScan(): replace SetVector with custom BitVectorVector

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D54418#1296093, @MaskRay wrote:

> Thanks for your benchmark file!
>
> `llvm-exegesis -mode=analysis -analysis-epsilon=100000 -benchmarks-file=/tmp/Downloads/benchmarks.yaml -analysis-inconsistencies-output-file=/tmp/clusters.html > /tmp/new`
>
> https://reviews.llvm.org/D54442 alone (without your other optimization) reduces the time from 16s to 4.2s on my machine. I'll leave the  `const auto Neighbors = rangeQuery(Q);` inlining and other stuff to you :)


After thinking, i see what the question was - why use abstractions when you can avoid using abstractions.
Well, because they are abstractions :) I have seen a bit much code, than i would have liked to, that intentionally avoided abstractions.
So i personally would not write everything inline like it is in https://reviews.llvm.org/D54442, and would not review that.
If it is abstracted, it is more readable, actually testable. And one day might even be moved to a more common (ADT e.g.) place should others need it.
If it is all inline, someone *will* have to refactor it to use abstractions later on.
So i guess the question boils down to - why bother writing good code when one can write "C spaghetti".

I'm not sure if https://reviews.llvm.org/D54442 is just a proof-of-concept or actually for the review,
but i would personally guess that small incremental patches
(with actual perf changes stated! since it is a perf improvements patchset) are better,
since small incremental changes are anyway recommended by the coding standards.


Repository:
  rL LLVM

https://reviews.llvm.org/D54418