Last Comment Bug 739532 - don't malloc BytecodeEmitter
: don't malloc BytecodeEmitter
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-26 23:33 PDT by Luke Wagner [:luke]
Modified: 2012-03-29 08:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.46 KB, patch)
2012-03-26 23:33 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-03-26 23:33:28 PDT
Created attachment 609628 [details] [diff] [review]
patch

It no longer seems necessary to malloc the BytecodeEmitter.  Bug 96526 (5 digits!) did it because, since js_EmitTree was one big function, the stack frame size determined, e.g., the max number of if/elses in a chain.

Bug 696284 comment 0 explains that stack allocation removes a bunch of mallocs.  I was personally annoyed by this since it means we have two different grep patterns to find all the places where we create BytecodeEmitters.

With the patch, I can still nest function f() { function f() { ... } } 221 deep, which seems plenty.  Nesting this deep is asking for other forms of pathological performance anyhow.
Comment 1 Nicholas Nethercote [:njn] 2012-03-27 01:51:43 PDT
> With the patch, I can still nest function f() { function f() { ... } } 221
> deep, which seems plenty.  Nesting this deep is asking for other forms of
> pathological performance anyhow.

Bug 696284 comment 4 said it that stack-allocating it reduced the max depth from 327 to 136... not sure if that was 32-bit or 64-bit.  Has something changed such that we don't have a similar reduction now?
Comment 2 Luke Wagner [:luke] 2012-03-27 03:05:13 PDT
Yes.  As comment 0 explained, js_EmitTree has been split up.  That is presumably why 221 > 136.  Regardless, it seems a bit silly to quibble about 200 vs. 300 levels of nesting although perhaps I am overlooking some way in which this matters...

If we actually cared about supporting deep nesting, it seems like we'd avoid the recursion and do something iterative.  (That sounds awful.)
Comment 3 Jason Orendorff [:jorendorff] 2012-03-27 09:57:33 PDT
Comment on attachment 609628 [details] [diff] [review]
patch

All right.
Comment 5 Matt Brubeck (:mbrubeck) 2012-03-29 08:41:06 PDT
https://hg.mozilla.org/mozilla-central/rev/1c7d12a257d0

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