Closed Bug 739532 Opened 12 years ago Closed 12 years ago

don't malloc BytecodeEmitter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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.
Attachment #609628 - Flags: review?(jorendorff)
> 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?
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 on attachment 609628 [details] [diff] [review]
patch

All right.
Attachment #609628 - Flags: review?(jorendorff) → review+
Assignee: general → luke
https://hg.mozilla.org/mozilla-central/rev/1c7d12a257d0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: