Bug 25674 – Syntax tree nodes should use arena allocation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

The syntax tree should be allocated out of an arena. This would make things a bit faster.

[Darin Adler]

Created attachment 30165 [details]
patch for part 1

[Sam Weinig]

Comment on attachment 30165 [details]
patch for part 1

> +        * parser/NodeConstructors.h: Updated for name change of parserObjects to
> +        parserArena. Also added explicit initialization for raw pointers that used
> +        to be RawPtr. Also removed some uses of get() that aren't needed now that

I think you mean, used to be "RefPtr".

r=me

[Darin Adler]

Comment on attachment 30165 [details]
patch for part 1

Clearing review flag since I landed this.

[Darin Adler]

Created attachment 30171 [details]
patch for part 2

[Cameron Zwarich (cpst)]

Comment on attachment 30171 [details]
patch for part 2

r=me, assuming performance is okay and you fix this typo:

    // <anually release as much as possible from the now-defunct declaration lists

I really like your work on this bug. I always wanted to do this, but the way you have broken it down into stages makes it much more palatable.

[Darin Adler]

Comment on attachment 30171 [details]
patch for part 2

Clearing review flag since I landed this one.

[Darin Adler]

Created attachment 30190 [details]
work in progress

[Darin Adler]

Created attachment 30257 [details]
work in progress

[Darin Adler]

Created attachment 30279 [details]
patch for part 3

[Cameron Zwarich (cpst)]

I am reviewing this.

[Cameron Zwarich (cpst)]

Comment on attachment 30279 [details]
patch for part 3

Typo:

"referenc count churn"

Other than that, r=me.

[Darin Adler]

http://trac.webkit.org/changeset/43642

I'm going to close this now. More refinement can be done, but we do have an arena at this point.

[Darin Adler]

Comment on attachment 30279 [details]
patch for part 3

The change measured as a regression, perhaps due to some late changes I made after testing myself. Had to roll it out.

http://trac.webkit.org/changeset/43661

[Darin Adler]

Rolled out so the bug is no longer fixed.

[Darin Adler]

Created attachment 35062 [details]
Latest version of patch for the remaining bit -- still can probably be broken in two

[Darin Adler]

Created attachment 35169 [details]
patch

[Gavin Barraclough]

Comment on attachment 35169 [details]
patch

Do objects of type ParserArenaFreeable & ParserArenaDeletable really get allocated outside of the arena? - if so, this makes me a little sad (or to put my question slightly more directly, do these classes really need subclass FastAllocBase, or was this just scaffolding?).

It also seems a little odd to provide two inline forms of makeIdentifier, which appear to be identical (since one calls the other), other than one being a member of the arena and the other taking the arena as an argument.  Am I missing something here? - since they appear interchangeable this just seems like something that could lead to inconsistency and confusion.

Otherwise all looks awesome, r+.
G.

[Darin Adler]

(In reply to comment #17)
> Do objects of type ParserArenaFreeable & ParserArenaDeletable really get
> allocated outside of the arena? - if so, this makes me a little sad (or to put
> my question slightly more directly, do these classes really need subclass
> FastAllocBase, or was this just scaffolding?).

At the time I originally wrote the patch there were indeed objects that get allocated outside the arena that inherit from freeable and deletable -- the ScopeNode objects for one.

At this time, though, that may no longer be true. I'll investigate.

> It also seems a little odd to provide two inline forms of makeIdentifier, which
> appear to be identical (since one calls the other), other than one being a
> member of the arena and the other taking the arena as an argument.  Am I
> missing something here? - since they appear interchangeable this just seems
> like something that could lead to inconsistency and confusion.

You are missing something, but it's not something major.

One of the function is a member of ParserArena, and the other of IdentifierArena. We can eliminate the makeIdentifier member functions of ParserArena and let all the calls go directly through the IdentifierArena. I'll fix that.

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/47571

[Darin Adler]

Mark Rowe had to roll out this patch because it was leaking the world!

[Darin Adler]

Looks like this landed in <http://trac.webkit.org/changeset/47664>.