Last Comment Bug 767013 - only store aliased variables in scope objects
: only store aliased variables in scope objects
Status: RESOLVED FIXED
[js:t][Memshrink:P3]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 765956 772285 772688 775323 780647 783441 783978
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 09:15 PDT by Luke Wagner [:luke]
Modified: 2012-08-20 03:16 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't store unaliased bindings (111.87 KB, patch)
2012-08-09 16:47 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-06-21 09:15:20 PDT
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).
Comment 1 Luke Wagner [:luke] 2012-06-22 09:28:23 PDT
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.)
Comment 2 Luke Wagner [:luke] 2012-06-22 12:35:10 PDT
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.
Comment 3 Nicholas Nethercote [:njn] 2012-06-22 19:33:56 PDT
> 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...
Comment 4 Luke Wagner [:luke] 2012-08-09 16:47:50 PDT
Created attachment 650722 [details] [diff] [review]
don't store unaliased bindings

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.
Comment 5 Brian Hackett (:bhackett) 2012-08-12 18:29:23 PDT
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?
Comment 6 Luke Wagner [:luke] 2012-08-13 09:20:03 PDT
(In reply to Brian Hackett (:bhackett) from comment #5)
'yes' on both, thanks!
Comment 8 Ed Morley [:emorley] 2012-08-16 06:16:48 PDT
https://hg.mozilla.org/mozilla-central/rev/abc8c217f032

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