Closed Bug 767013 Opened 12 years ago Closed 12 years ago

only store aliased variables in scope objects

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: [js:t][Memshrink:P3])

Attachments

(1 file)

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: general → luke
Whiteboard: [js:t]
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.)
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]
Depends on: 765956
> 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...
Whiteboard: [js:t][Memshrink] → [js:t][Memshrink:P3]
Depends on: 772285
Depends on: 772688
Depends on: 775323
Depends on: 780647
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 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+
(In reply to Brian Hackett (:bhackett) from comment #5)
'yes' on both, thanks!
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.