[PATCH] D126723: [pseudo] GC GSS nodes, reuse them with a freelist

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Nice! I really like the form it goes now.



================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:82
 
     // FIXME: Most nodes live a fairly short time, and are simply discarded.
     // Is it worth refcounting them (we have empty padding) and returning to a
----------------
This FIXME is done by this patch :)


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:15
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
----------------
an off-topic comment: we only use the function in debug branch code, this will trigger the include-cleaner warning in release mode :)

we could warp it with `#ifndef NDEBUG` as well.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:87
     NewHeads.clear();
+    MaybeGC();
     glrReduce(PendingReduce, Params,
----------------
I would just call it before the `NewHeads.clear()`, and run the `gc` on the `NewHeads` directly.

It simplifies the implementation (no need to traverse three pending actions, and no duplicated elements in Roots). The downside is that some dead heads (heads with no available actions on `Terminal.symbol()`) will not be GCed in this run, but I think it is negligible, as those will be GCed in next run.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:391
+                              llvm::ArrayRef<const Node *> Parents) {
+  ++NodeCount;
+  Node *Result = new (allocate(Parents.size()))
----------------
The NodeCount becomes vague now, we update in both addNode and allocate methods, I think we should have two separate counts.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:403
+  ++NodeCount;
+  if (FreeList.size() <= Parents)
+    FreeList.resize(Parents + 1);
----------------
What's the practical size of the `FreeList`? I think we can just find one, and use it as initial default size, to save some cost of resize. 


================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:412
+  EXPECT_EQ(0u, GSStack.gc({AB, C})); // D is already gone.
+  auto *E = GSStack.addNode(0, nullptr, {Root});
+  EXPECT_EQ(3u, GSStack.gc({A, E})); // destroys B, AB, C
----------------
nit: since we free D, I think here E will reuse the memory free from D, maybe add pointer comparison with `E` and `D` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126723