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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits