Closed
Bug 739532
Opened 12 years ago
Closed 12 years ago
don't malloc BytecodeEmitter
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(1 file)
3.46 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter 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)
Comment 1•12 years ago
|
||
> 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?
Assignee | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 609628 [details] [diff] [review] patch All right.
Attachment #609628 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7d12a257d0
Target Milestone: --- → mozilla14
Updated•12 years ago
|
Assignee: general → luke
Comment 5•12 years ago
|
||
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.
Description
•