sammccall marked an inline comment as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:391
+ llvm::ArrayRef<const Node *> Parents) {
+ ++NodeCount;
+ Node *Result = new (allocate(Parents.size()))
----------------
hokein wrote:
> sammccall wrote:
> > 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.
> > names are vague now, both "NodeCount" and "Allocated" are dubious. Maybe
> > I'll rename to NodesCreated and Alive, WDYT?
>
> Sounds good to me.
>
> > 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.
>
> I think these are sufficient, should provide enough details about
> GSS-memory-related information. The other things I will like to record is the
> max-active-heads, but that's a different bit.
>
> > do we want to expose any of those numbers through APIs? I couldn't work out
> > what I'd actually do with them.
>
> I think we should avoid to expose an API per field, we probably just need a
> single API which returns a Stats struct containing all these things. We can
> do it later.
>
>
OK, I haven't added any new stats APIs for now.
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:403
+ ++NodeCount;
+ if (FreeList.size() <= Parents)
+ FreeList.resize(Parents + 1);
----------------
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 :-)
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
