If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Propagating shape mutations along the parent chain only for Call objects

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
enhancement
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug, {fixed1.9.1})

Trunk
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Currently for all delegated objects a scope chain mutation is propagated not only along the prototype chain but also along the parent chain. This means that the shape of the global object is mutated in almost all cases as long as any delegated object is mutated.

With the bug 487039 fixed this is not necessary in general since name lookups only for white-listed non-globals are cached. Moreover, among 3 white-listed classes only a Call object can gain new properties. Thus it is necessary to propagate the shape mutations only for Call objects.
(Assignee)

Updated

9 years ago
Blocks: 460050
(Assignee)

Comment 1

9 years ago
http://hg.mozilla.org/tracemonkey/rev/9caa852c758c
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 2

9 years ago
(In reply to comment #1)
> http://hg.mozilla.org/tracemonkey/rev/9caa852c758c

Ignore this, this is for the bug 492659.
Whiteboard: fixed-in-tracemonkey
More comments for this bug are in bug 460050 comment 35 onward.

/be
Blocking since bug 460050 blocks and this is a necessary if not sufficient bug to fix for that bug to be non-blocking.

/be
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+

Comment 5

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9caa852c758c
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Rob, see comment 2 -- the hgweb link in comment 1 is for bug 490364.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED

Updated

9 years ago
Priority: -- → P2

Comment 7

9 years ago
(In reply to comment #6)
> Rob, see comment 2 -- the hgweb link in comment 1 is for bug 490364.

Ah, got it.
(Assignee)

Comment 8

9 years ago
Created attachment 377418 [details] [diff] [review]
v1 (work in progress)

The patch propagates the shape changes along the parent chain only for call objects and adds an assert that any cacheable non-global can gain property only in a controlled way via js_DefineNativeProperty. I hope this should be enough to offset the slowdown in the go benchmark from the bug 460050.

The patch is not tested.
(Assignee)

Comment 9

9 years ago
(In reply to comment #8)
> I hope this should be enough
> to offset the slowdown in the go benchmark from the bug 460050.

This is wrong - the patch still propagates shape changes for Call objects along the parent chain, the primary source of the slowdown for the GO test case.
(Assignee)

Comment 10

9 years ago
Created attachment 377457 [details] [diff] [review]
v2

The new version incorporates the idea from the bug 460050 about never purging the Call object scope chains for locals. In particular, it follows the suggestion from the bug 460050 comment 36.

This passes the shell tests and I am about to submit it to the try server.

But the bad news is that the go benchmark from the bug 460050 comment #5 is not affected one bit by this changes. The reason for that is simple: the code there never triggers call_resolve for locals. So 3% slowdown with that test case with the jit that I reliably see is still there.
Attachment #377418 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #377457 - Flags: review?(brendan)
I can confirm that this doesn't really have much of an effect on bug 460050...   I wonder why; I definitely saw us aborting with global shape changes there.  Maybe we're still hitting other aborts even with this patch?
Comment on attachment 377457 [details] [diff] [review]
v2

>+    /*
>+     * We must purge the scope chain only for Call objects as it the only
>+     * cacheable non-global object that can gain properties after been cached
>+     * via eval calls introducing new vars, see bug 490364.

Nits, suggest this rewrite:

     * We must purge the scope chain only for Call objects as they are the only
     * kind of cacheable non-global object that can gain properties after outer
     * properties with the same names have been cached or traced. Call objects
     * may gain such properties via eval introducing new vars; see bug 490364.

[call_resolve:]
>+    /*
>+     * Check if the id refers to a formal parameter, local variable or the
>+     * arguments special name.

s/if the id/whether id/

>+     *
>+     * We define all such names using JSDNP_DONT_PURGE to avoid an expensive
>+     * shape invalidation in js_DefineNativeProperty. If such name happens to

s/such name/such an id/

>+     * shadow a global or upvar of the same name, the inner binding can never

s/the inner binding/any inner code/ or perhaps /inner functions/

>+     * access the outer binding. Thus it cannot invalidate any property cache
>+     * entries or derived trace guards for the outer binding. Only bindings
>+     * that eval creates in a Call object could happen after a reference to a
>+     * free or upvar was evaluated. See bug 460050.

Your point about too much duplication among comments from bug 492355 applies here and earlier. One canonical explanatory comment seems best, the rest can refer to it by fuzzy source coordinates, if necessary.

r=me with these nits picked, thanks.

/be
Attachment #377457 - Flags: review?(brendan) → review+
(Assignee)

Comment 13

9 years ago
Created attachment 377651 [details] [diff] [review]
v3

In the new patch I fixed the comments as suggested above. It was a good suggestion to follow my own advice to avoid commenting on the same issue in different places.
Attachment #377457 - Attachment is obsolete: true
Attachment #377651 - Flags: review+
(Assignee)

Comment 14

9 years ago
http://hg.mozilla.org/tracemonkey/rev/df4b09b2e5a0
Whiteboard: fixed-in-tracemonkey

Comment 15

9 years ago
http://hg.mozilla.org/mozilla-central/rev/df4b09b2e5a0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 16

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0076fe7236b6
Keywords: fixed1.9.1

Comment 17

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c01ead820fda
You need to log in before you can comment on or make changes to this bug.