696284 - Add a comment explaining why we allocate BytecodeEmitter on the heap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: 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]]

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 [inactive]]

(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]]

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]]

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 [inactive]]

Attachment: 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]]

Comment on attachment 568982 [details] [diff] [review]
patch with just a comment

Yes. Thanks.

Comment 7

[Nicholas Nethercote [inactive]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8df7a753bea0

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/8df7a753bea0