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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:78
+    // We need to copy the list: Roots is consumed by the GC.
+    Roots = NewHeads;
+    GSS.gc(std::move(Roots));
----------------
nit: I'd rather pass the NewHeads as a vector parameter, and get rid of the `Roots` variable in the lambda, but up to you.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:403
+  ++NodeCount;
+  if (FreeList.size() <= Parents)
+    FreeList.resize(Parents + 1);
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > 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!
> > I would image there will be multiple resize calls, if the path like `allocate(1), allocate(3), ..., allocate(max)`, which I think it is practical? unless the first call is `allocate(max)` which is unlikely I think.
> So for parsing clangd/AST.cpp, we have 4 small allocations for the whole file, which we could reduce by 3.
> ```
> size increased to 1
> capacity grew from 0 to 1
> size increased to 2
> capacity grew from 1 to 2
> size increased to 3
> capacity grew from 2 to 4
> size increased to 4
> size increased to 6
> capacity grew from 4 to 8
> size increased to 7
> ```
> 
> By comparison, **each call** to glrReduce creates 5 vectors and 2 priority queues, each of which can have multiple allocations as it grows. There are 4000 tokens, for a total of probably **50000** mallocs.
> 
> I think you're microoptimizing the wrong places :-)
that's fair enough (and there are some opportunities to optimize the glrReduce). But I think adding a single `FreeList.revese(10);` statement seems trivial. Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126723