don't malloc BytecodeEmitter

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
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?
(Assignee)

Comment 2

6 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 on attachment 609628 [details] [diff] [review]
patch

All right.
Attachment #609628 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7d12a257d0
Target Milestone: --- → mozilla14

Updated

6 years ago
Assignee: general → luke
https://hg.mozilla.org/mozilla-central/rev/1c7d12a257d0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.