[PATCH] D54442: [llvm-exegesis] Optimize ToProcess in dbScan

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Clustering.cpp:120
     // Process P's neighbors.
-    llvm::SetVector<size_t, std::deque<size_t>> ToProcess;
-    ToProcess.insert(Neighbors.begin(), Neighbors.end());
-    while (!ToProcess.empty()) {
+    size_t Tail = 0;
+    for (size_t Q : Neighbors)
----------------
courbet wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > lebedev.ri wrote:
> > > > courbet wrote:
> > > > > lebedev.ri wrote:
> > > > > > MaskRay wrote:
> > > > > > > courbet wrote:
> > > > > > > > All of this makes the dbscan implementation harder to follow. Please move all this to a separate struct:
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > // This is long-lived.
> > > > > > > > struct WorkSetStorage {
> > > > > > > >   public:
> > > > > > > >    WorkSetStorage(size_t NumPoints) : ToProcess(NumPoints), Added(NumPoints) {}
> > > > > > > > 
> > > > > > > >    void setProcessed(size_t P) { Added[P] = 1; }
> > > > > > > > 
> > > > > > > >   private:
> > > > > > > >    friend class WorkSet;
> > > > > > > >    std::vector<size_t> ToProcess;
> > > > > > > >    std::vector<char> Added;
> > > > > > > > };
> > > > > > > > 
> > > > > > > > // This is short-lived and replaces  llvm::SetVector<size_t, std::deque<size_t>> ToProcess;
> > > > > > > > class WorkSet {
> > > > > > > >   public:
> > > > > > > >    WorkSet(WorkSetStorage& Storage) : Storage(Storage), Head(0), Tail(0) {}
> > > > > > > > 
> > > > > > > >    // Inserts a point if not already processed.
> > > > > > > >    void insert(size_t P);
> > > > > > > > 
> > > > > > > >    // returns the first element from the work set and pops it.
> > > > > > > >    size_t pop();
> > > > > > > > 
> > > > > > > >   private:
> > > > > > > >    WorkSetStorage& Storage;
> > > > > > > >    size_t Head;
> > > > > > > >    size_t Tail;
> > > > > > > > };
> > > > > > > > ```
> > > > > > > Sorry I don't think making the abstraction is very necessary. The use of `Head/Tail` is very local and takes very few lines of code. The dbscan algorithm is in essence a variant of Dijkstra or FIFO Bellman-Ford. There is not much fantasy here. Keeping these lines inline is IMHO more readable.
> > > > > > (FWIW i have initially implemented this in D54418 with (but different) abstractions, before this review was submitted)
> > > > > The abstraction makes it clear that we're working with a set. I'm no saying we should have exactly this abstraction (actually D54418 had another approach), but clearly no abstraction makes it harder to read at least for //me//...
> > > > I can reopen that one, or change it to such manual deque.
> > > I have said I don't think the abstraction is necessary. It just makes a simple algorithm complicated and harder to understand as developers have to read 2 separate places for the single usage.
> > Gentle ping.
> > 
> > Please take another look. I've even simplified the logic. The additional abstraction (which will be one-shot and takes tens of lines) will assuredly make the coder harder to read.
> Let me explain my rationale: As a developer, I read the code, the abstraction says it has set semantics, I don't read the implementation and carry on with reading the code keeping in mind that this is a set. I'll only look at the actual implementation if I want to understand more. With the implementation in this patch the set semantics are not obvious.
> 
> The pattern:
> ```
> while (!set.empty()) {
>   auto elem = set.top();
>   set.pop();
>   ...
> }
> ```
> 
> will be *extremely* familiar to anyone. 
> 
> On the other hand, in:
> ```
> for (size_t Head = 0; Head < Tail; ++Head) {
>   P = ToProcess[Head];
> }
> ```
> 
> it's not trivial what's happening.
> As a developer, I read the code, the abstraction says it has set semantics, I don't read the implementation and carry on with reading the code keeping in mind that this is a set.

Thanks for expressing your argument. I would say the existing style may not be familiar to users as a point may be added to the queue multiple times, while the patch changes it to a strictly BFS manner.

> for (size_t Head = 0; Head < Tail; ++Head) {
>  P = ToProcess[Head];
> }

The for loop is not a foreign BFS style coined by me. It is well established and also used otherwhere.

https://github.com/llvm-mirror/llvm/tree/master/lib/Target/X86/X86ISelLowering.cpp#L18472
https://github.com/llvm-mirror/llvm/tree/master/lib/Transforms/Utils/LoopUtils.cpp#L470
https://github.com/llvm-mirror/llvm/tree/master/lib/CodeGen/LiveRangeCalc.cpp#L360




Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54442/new/

https://reviews.llvm.org/D54442