Last Comment Bug 696284 - Add a comment explaining why we allocate BytecodeEmitter on the heap
: Add a comment explaining why we allocate BytecodeEmitter on the heap
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-20 17:53 PDT by Nicholas Nethercote [:njn]
Modified: 2011-11-03 08:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.93 KB, patch)
2011-10-20 17:53 PDT, Nicholas Nethercote [:njn]
jorendorff: review-
Details | Diff | Review
patch with just a comment (1.03 KB, patch)
2011-10-23 16:29 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2011-10-20 17:53:13 PDT
Created attachment 568574 [details] [diff] [review]
patch

EmitTree allocates a CodeGenerator on the heap.  It can easily be allocated on the stack instead.  The simple attached patch does this.

The patch creates a new block to provide the exact same lifetime for the CodeGenerator that we had previously.  I generated the patch with 'hg qdiff -w' because of the indentation change.

If I start the browser and load Gmail and TechCrunch, this patch avoids 64,728
out of 4,377579 calls to malloc.  That's a 1.5% reduction, not bad!
Comment 1 Brendan Eich [:brendan] 2011-10-20 18:20:54 PDT
See bug 96526 -- cg2 used to be a stack allocated struct, we changed to avoid blowing the stack limit prematurely. Do gcc et al. correctly avoid overallocating js::EmitTree's frame and instead adjust the stack to allocate cg2 only in its block?

/be
Comment 2 Nicholas Nethercote [:njn] 2011-10-20 18:32:23 PDT
(In reply to Brendan Eich [:brendan] from comment #1)
> See bug 96526 -- cg2 used to be a stack allocated struct, we changed to
> avoid blowing the stack limit prematurely.

Sure would have been nice to have that in a comment.

Nonetheless, I did think about this issue and did some measurement.  In a small amount of browsing I didn't see a depth greater than 6.  On the test case in bug 96526 I get a depth of 1.

> Do gcc et al. correctly avoid
> overallocating js::EmitTree's frame and instead adjust the stack to allocate
> cg2 only in its block?

I don't know.  I was more interested in having the destructor run at the same time, less worried about when the stack memory disappeared.
Comment 3 Brendan Eich [:brendan] 2011-10-20 18:56:06 PDT
Yes, the tail recursion still kicks in. The question is (apart from why I didn't comment the change -- figured you'd cvs blame if you cared :-P), what is the size of EmitTree's frame on top platforms before and after this bug's patch.

It could matter, I'm not here for fun! We'd have to find a deep recursive path. The testsuite may have one. Does it pass?

/be
Comment 4 Jason Orendorff [:jorendorff] 2011-10-21 11:19:41 PDT
Comment on attachment 568574 [details] [diff] [review]
patch

On my machine, this patch reduces the max depth of nested functions we can compile from 327 to 136.

It's admittedly weird that the one affects the other, but that's where we are. We can't take this. r-.
Comment 5 Nicholas Nethercote [:njn] 2011-10-23 16:29:19 PDT
Created attachment 568982 [details] [diff] [review]
patch with just a comment

My rule of thumb for comments is that if it's non-obvious, it deserves a comment.
Comment 6 Jason Orendorff [:jorendorff] 2011-11-02 15:00:47 PDT
Comment on attachment 568982 [details] [diff] [review]
patch with just a comment

Yes. Thanks.
Comment 7 Nicholas Nethercote [:njn] 2011-11-02 20:55:08 PDT
Note that |CodeGenerator| was renamed |BytecodeEmitter| before this patch landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8df7a753bea0
Comment 8 Marco Bonardo [::mak] 2011-11-03 08:53:32 PDT
https://hg.mozilla.org/mozilla-central/rev/8df7a753bea0

Note You need to log in before you can comment on or make changes to this bug.