The default bug view has changed. See this FAQ.

Add a comment explaining why we allocate BytecodeEmitter on the heap

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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!
Attachment #568574 - Flags: review?(jorendorff)
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
(Assignee)

Comment 2

6 years ago
(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.
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 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-.
Attachment #568574 - Flags: review?(jorendorff) → review-
(Assignee)

Updated

6 years ago
Summary: Don't allocate CodeGenerator on the heap → Add a comment explaining why we allocate CodeGenerator on the heap
(Assignee)

Comment 5

6 years ago
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.
Attachment #568574 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #568982 - Flags: review?(jorendorff)
Comment on attachment 568982 [details] [diff] [review]
patch with just a comment

Yes. Thanks.
Attachment #568982 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

6 years ago
Note that |CodeGenerator| was renamed |BytecodeEmitter| before this patch landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8df7a753bea0
Summary: Add a comment explaining why we allocate CodeGenerator on the heap → Add a comment explaining why we allocate BytecodeEmitter on the heap
https://hg.mozilla.org/mozilla-central/rev/8df7a753bea0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.