Bug 25674 - Syntax tree nodes should use arena allocation
Summary: Syntax tree nodes should use arena allocation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-10 13:21 PDT by Darin Adler
Modified: 2011-11-01 17:05 PDT (History)
3 users (show)

See Also:


Attachments
patch for part 1 (264.11 KB, patch)
2009-05-10 13:37 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch for part 2 (54.44 KB, patch)
2009-05-10 20:57 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
work in progress (28.38 KB, patch)
2009-05-11 09:32 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
work in progress (65.88 KB, patch)
2009-05-12 17:53 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch for part 3 (131.99 KB, patch)
2009-05-13 10:30 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Latest version of patch for the remaining bit -- still can probably be broken in two (65.05 KB, patch)
2009-08-18 12:36 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch (56.67 KB, patch)
2009-08-19 18:04 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2009-05-10 13:21:28 PDT
The syntax tree should be allocated out of an arena. This would make things a bit faster.
Comment 1 Darin Adler 2009-05-10 13:37:21 PDT
Created attachment 30165 [details]
patch for part 1
Comment 2 Sam Weinig 2009-05-10 13:49:50 PDT
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
Comment 3 Darin Adler 2009-05-10 15:33:39 PDT
Comment on attachment 30165 [details]
patch for part 1

Clearing review flag since I landed this.
Comment 4 Darin Adler 2009-05-10 20:57:57 PDT
Created attachment 30171 [details]
patch for part 2
Comment 5 Cameron Zwarich (cpst) 2009-05-10 21:24:06 PDT
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.
Comment 6 Darin Adler 2009-05-10 21:36:35 PDT
Comment on attachment 30171 [details]
patch for part 2

Clearing review flag since I landed this one.
Comment 7 Darin Adler 2009-05-11 09:32:01 PDT
Created attachment 30190 [details]
work in progress
Comment 8 Darin Adler 2009-05-12 17:53:30 PDT
Created attachment 30257 [details]
work in progress
Comment 9 Darin Adler 2009-05-13 10:30:20 PDT
Created attachment 30279 [details]
patch for part 3
Comment 10 Cameron Zwarich (cpst) 2009-05-13 10:35:51 PDT
I am reviewing this.
Comment 11 Cameron Zwarich (cpst) 2009-05-13 10:39:18 PDT
Comment on attachment 30279 [details]
patch for part 3

Typo:

"referenc count churn"

Other than that, r=me.
Comment 12 Darin Adler 2009-05-13 11:16:28 PDT
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.
Comment 13 Darin Adler 2009-05-13 14:56:39 PDT
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
Comment 14 Darin Adler 2009-05-18 14:02:40 PDT
Rolled out so the bug is no longer fixed.
Comment 15 Darin Adler 2009-08-18 12:36:49 PDT
Created attachment 35062 [details]
Latest version of patch for the remaining bit -- still can probably be broken in two
Comment 16 Darin Adler 2009-08-19 18:04:06 PDT
Created attachment 35169 [details]
patch
Comment 17 Gavin Barraclough 2009-08-19 19:54:05 PDT
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.
Comment 18 Darin Adler 2009-08-19 21:57:01 PDT
(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.
Comment 19 Eric Seidel (no email) 2009-08-21 14:50:35 PDT
http://trac.webkit.org/changeset/47571
Comment 20 Darin Adler 2009-08-21 17:22:15 PDT
Mark Rowe had to roll out this patch because it was leaking the world!
Comment 21 Darin Adler 2011-11-01 17:05:37 PDT
Looks like this landed in <http://trac.webkit.org/changeset/47664>.