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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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)
----------------
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)


Repository:
  rL LLVM

https://reviews.llvm.org/D54442