Closed Bug 490364 Opened 15 years ago Closed 15 years ago

Propagating shape mutations along the parent chain only for Call objects

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 460050
http://hg.mozilla.org/tracemonkey/rev/9caa852c758c
Whiteboard: fixed-in-tracemonkey
(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+
http://hg.mozilla.org/mozilla-central/rev/9caa852c758c
Status: NEW → RESOLVED
Closed: 15 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
Priority: -- → P2
(In reply to comment #6)
> Rob, see comment 2 -- the hgweb link in comment 1 is for bug 490364.

Ah, got it.
Attached patch v1 (work in progress) (obsolete) — Splinter Review
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.
(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.
Attached patch v2 (obsolete) — Splinter Review
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
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+
Attached patch v3Splinter Review
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+
http://hg.mozilla.org/tracemonkey/rev/df4b09b2e5a0
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/df4b09b2e5a0
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.