[PATCH] D54442: [llvm-exegesis] Optimize ToProcess in dbScan
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 14 02:22:51 PST 2018
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)
----------------
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.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54442/new/
https://reviews.llvm.org/D54442
More information about the llvm-commits
mailing list