Closed
Bug 767013
Opened 12 years ago
Closed 12 years ago
only store aliased variables in scope objects
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: [js:t][Memshrink:P3])
Attachments
(1 file)
111.87 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
Even after bug 659577, scope objects still have a slot for every variable, aliased or unaliased. This was a simplification to avoid having two different variable indexing schemes. David and Brian pointed out that this is hurting us on v8 which has scopes with many unaliased variables and few aliased variables (which causes us to overflow the 16-slot inline limit and require malloc'd slots). Fortunately, I think the existing ScopeCoordinate and helpers abstract variable indexing well enough that this shouldn't be too hard to fix. There is one small bump: the unaliased variables in scope objects actually *are* accessed: they hold the values of unaliased variables after the frame has been popped so that the debugger can still read/write to them (grep DONT_CHECK_ALIASING in ScopeObject.cpp). However, I think we can just stick these unaliased variables on the DebugScopeProxy instead (in a private malloc'd byte array).
![]() |
Assignee | |
Updated•12 years ago
|
Assignee: general → luke
Updated•12 years ago
|
Whiteboard: [js:t]
![]() |
Assignee | |
Comment 1•12 years ago
|
||
Arg, I forgot about a third complication: bindings are added for each variable eagerly during parsing, before we can know whether the variable gets a slot or not. I think the solution is to not create bindings until we finish the function (and thus know which bindings are aliased). (Reason #10 why scope objects should not be JSObjects.)
![]() |
Assignee | |
Comment 2•12 years ago
|
||
Actually, with the change in comment 1, we could just not create bindings for unaliased variables. The only code now that would notice would be the debugger, so it seems like we'd just need a way to stash away an array of atoms (names of unaliased vars) so the debugger could recover them. Nick: in bug 678037 comment 23, I mentioned using bindings to resolve upvars with lazy compilation which would, in general, need to know about unaliased bindings. However, the first pass should have marked variables as aliased such that the second pass, by construction, wouldn't ever ask about unaliased bindings. Sound right? I did a quick hack to about:memory to sum up the total memory spent on shapes created for bindings. On startup, I see .5MB (1% of total explicit), with 4 tabs (TC, tbpl, gmail, zimbra) I see 1.6MB (.7% total explicit), so this could put a nice dent.
Whiteboard: [js:t] → [js:t][Memshrink]
![]() |
||
Comment 3•12 years ago
|
||
> Nick: in bug 678037 comment 23, I mentioned using bindings to resolve upvars
> with lazy compilation which would, in general, need to know about unaliased
> bindings. However, the first pass should have marked variables as aliased
> such that the second pass, by construction, wouldn't ever ask about
> unaliased bindings. Sound right?
Sounds plausible. I think you understand more about that than I do at the moment...
![]() |
||
Updated•12 years ago
|
Whiteboard: [js:t][Memshrink] → [js:t][Memshrink:P3]
![]() |
Assignee | |
Comment 4•12 years ago
|
||
On v8, this is a 500 point improvement (12200 to 12700). On IM (where call object creation is inlined), hopefully this will be a bigger improvement. To wit: after the patch, there is still one function in earley-boyer that needs to malloc slots (the one with sc_optrOpnd_20). Before the patch, the CallObject would store 53 locals, but of those 18 are aliased. Only 1000 are created, so not a big deal.obj->slots.
Attachment #650722 -
Flags: review?(bhackett1024)
Comment 5•12 years ago
|
||
Comment on attachment 650722 [details] [diff] [review] don't store unaliased bindings Review of attachment 650722 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/jsinferinlines.h @@ +523,5 @@ > > if (script->length >= 50) > return false; > > + if (script->hasConsts() || script->hasObjects() || script->hasRegexps()) || script->function()->isHeavyweight() ? ::: js/src/jsscript.h @@ +92,5 @@ > + public: > + explicit Binding() : bits_(0) {} > + > + Binding(PropertyName *name, BindingKind kind, bool aliased) { > + JS_ASSERT((size_t(name) & 0x7) == 0); Can you use constants for these masks and JS_STATIC_ASSERT that BindingKind fits in 0x3?
Attachment #650722 -
Flags: review?(bhackett1024) → review+
![]() |
Assignee | |
Comment 6•12 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #5) 'yes' on both, thanks!
![]() |
Assignee | |
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc8c217f032
Target Milestone: --- → mozilla17
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/abc8c217f032
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
•