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!
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
(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-.
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 on attachment 568982 [details] [diff] [review] patch with just a comment Yes. Thanks.
Note that |CodeGenerator| was renamed |BytecodeEmitter| before this patch landed. https://hg.mozilla.org/integration/mozilla-inbound/rev/8df7a753bea0