only store aliased variables in scope objects

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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

5 years ago
Assignee: general → luke
Whiteboard: [js:t]
(Assignee)

Comment 1

5 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

5 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]
(Assignee)

Updated

5 years ago
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]
(Assignee)

Updated

5 years ago
Depends on: 772285
(Assignee)

Updated

5 years ago
Depends on: 772688
(Assignee)

Updated

5 years ago
Depends on: 775323
(Assignee)

Updated

5 years ago
Depends on: 780647
(Assignee)

Comment 4

5 years ago
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.
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+
(Assignee)

Comment 6

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #5)
'yes' on both, thanks!
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc8c217f032
Target Milestone: --- → mozilla17

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/abc8c217f032
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 783441
Depends on: 783978
You need to log in before you can comment on or make changes to this bug.