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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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"
----------------
hokein wrote:
> 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.
conditional including is ugly and can cause subtle opt-only compile failures.
I'm tempted to add a "keep" pragma but want to think about how to handle this case a bit more.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:87
     NewHeads.clear();
+    MaybeGC();
     glrReduce(PendingReduce, Params,
----------------
hokein wrote:
> 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.
Oops, I forgot that Pending* are all empty at the beginning of this loop. I should probably add an assertion for that.

We can call it right at the top of the loop, yes? It doesn't need to be between AddSsteps and NewHeads.clear().

> run the gc on the NewHeads directly
Unfortunately not: the GC naturally consumes the argument (it becomes the stack for the DFS) so we need a copy.
We could move the copy into gc() but:
 - this way we can reuse the same vector every time and don't have to allocate
 - soon we'll have error-recovery candidates as additional roots we want to pass in


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:391
+                              llvm::ArrayRef<const Node *> Parents) {
+  ++NodeCount;
+  Node *Result = new (allocate(Parents.size()))
----------------
hokein wrote:
> The NodeCount becomes vague now, we update in both addNode and allocate methods, I think we should have two separate counts.
- Oops, that duplicated ++NodeCount is just a mistake.
- names are vague now, both "NodeCount" and "Allocated" are dubious. Maybe I'll rename to `NodesCreated` and `Alive`, WDYT?
- are there actually more metrics we want to record? we have nodes-created, currently-live-nodes (Allocated.size()), nodes-destroyed (`NodeCount - Allocated.size()`). We could track max-live-node but I think Arena.bytes() is fine for our purposes.
- do we want to expose any of those numbers through APIs? I couldn't work out what I'd actually do with them.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:403
+  ++NodeCount;
+  if (FreeList.size() <= Parents)
+    FreeList.resize(Parents + 1);
----------------
hokein wrote:
> 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. 
It's whatever the max #parents is - maybe 10 or something?
I don't think it's worth adding a hard-coded guess to save ~1 resize of a single vector per parse!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126723