490364 - Propagating shape mutations along the parent chain only for Call objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/9caa852c758c

Comment 2

[Igor Bukanov]

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

Ignore this, this is for the bug 492659.

Comment 3

[Brendan Eich [:brendan]]

More comments for this bug are in bug 460050 comment 35 onward.

/be

Comment 4

[Brendan Eich [:brendan]]

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

/be

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9caa852c758c

Comment 6

[Brendan Eich [:brendan]]

Rob, see comment 2 -- the hgweb link in comment 1 is for bug 490364.

/be

Comment 7

[Robert Sayre]

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

Ah, got it.

Comment 8

[Igor Bukanov]

Attachment: 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.

Comment 9

[Igor Bukanov]

(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.

Comment 10

[Igor Bukanov]

Attachment: 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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 12

[Brendan Eich [:brendan]]

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

Comment 13

[Igor Bukanov]

Attachment: 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.

Comment 14

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/df4b09b2e5a0

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/df4b09b2e5a0

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0076fe7236b6

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c01ead820fda