408143 - JSObjects created by XPConnect don't share scopes.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Johnny Stenback (:jst)]

Attachment: Share protos if the ops differ, as long as the newObjectMap hooks are the same.

There's no reason JSObjects created by XPConnect shouldn't initially share scopes (maps) with their prototypes, for the last 7 or so years they haven't been, but for no good reason. The reason for not sharing is that the class and prototype have different ops, but we really should only not share when the newObjectMap hook in the ops differ (per Brendan). See attached patch.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 292903 [details] [diff] [review]
Share protos if the ops differ, as long as the newObjectMap hooks are the same.

>+     * Share proto's map only if it has the same newObjectMap ops,
>+     * and only if proto's class has the same private and reserved
>+     * slots as obj's map and class have.  We assume that if prototype
>+     * and object are of the same class, they always have the same
>+     * number of computed reserved slots (returned via
>+     * clasp->reserveSlots); otherwise, prototype and object classes
>+     * must have the same (null or not) reserveSlots hook.

Nit: might wrap better, and map to the code the comment documents, if you s/prototype and object/proto and obj/ in the second sentence. r/a=me with that, and thanks to you -- and to pav for finding this ancient perf/footprint bug.

/be

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #0)
> The reason for not sharing is that the class and
> prototype have different ops, but we really should only not share when the
> newObjectMap hook in the ops differ (per Brendan). See attached patch.

Used to be different JSObjectOps => different scopes, so obj and proto could not share via obj->map and safely downcast. jband made the condition narrower when he did XPConnect, varying certain custom ops but keeping JSObjectOps.newObjectMap (which allocates a map). If that hook matches between two objects, they must have the same-shaped obj->map, so can share. But this code was missed, or did not exist in this form, back then -- and it was never updated!

This could be huge.

/be

Comment 3

[Johnny Stenback (:jst)]

Fix checked in.

Comment 4

[Brendan Eich [:brendan]]

Had to back out due to mochitest failures:

js/src/jsobj.c 3.406

sayrer: any details from the logs you can lay on us, or diagnostic clues, would be much appreciated.

jst: I'll help fix tomorrow when you get in.

/be

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The same tests failed on all three platforms (with the exception of the chrome mochitests on Windows, because they arent' run).

Per http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1197524820.1197527233.28475.gz&fulltext=1 , the failing tests are:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/test/test_bug372769.xhtml
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/tests/mochitest/bugs/test_bug370098.html
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/tests/chrome/test_bug253481.xul
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/test/browser_bug321000.js

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> Used to be different JSObjectOps => different scopes, so obj and proto could
> not share via obj->map and safely downcast. jband made the condition narrower
> when he did XPConnect, varying certain custom ops but keeping
> JSObjectOps.newObjectMap (which allocates a map). If that hook matches between
> two objects, they must have the same-shaped obj->map, so can share.

I'm a bit confused as to how this patch could work: if the ops are different, but newObjectMap is the same then the map will be shared but the wrong (now shared) ops will be used, because the ops are held by the map?

Comment 7

[Johnny Stenback (:jst)]

Yeah, this was clearly not the right approach here. Unless we want to take significant changes to the JS engine, I think what we need here is to make XPConnect expose the same JSObjectOps for its wrappers and classes, and then make the actual ops detect for which class they're called and do the right thing. There's only a couple of actual ops (enumerate, clear, call, and construct, none of which are really performance critical), so the overhead there should be far worth not having to create new scopes for every wrapper.

Comment 8

[Brendan Eich [:brendan]]

We were a bit giddy last night (elf vs. dwarf malloc-killing contest, had to beat sicking-gimli).

jst: sounds good, should shrink code and initialized data size too.

/be

Comment 9

[Peter Van der Beken [:peterv]]

I'll gladly blame sicking!

(In reply to comment #7)
> I think what we need here is to make
> XPConnect expose the same JSObjectOps for its wrappers and classes

You mean have one set of ops for all XPConnect JSClasses?

Comment 10

[Brendan Eich [:brendan]]

The important ops to unify are prototype vs. instance -- XPCWrappedNativeProto vs XPCWrappedNative. There may be others like that, but just that unification will dramatically reduce the number of eagerly created JSScopes, almost all of which are never needed (no expandos set on any instance).

/be

Comment 11

[Johnny Stenback (:jst)]

(In reply to comment #9)
> You mean have one set of ops for all XPConnect JSClasses?

XPConnect uses two different JSObjectOps for XPCWrappedNatives, 
XPC_WN_NoCall_JSOps and XPC_WN_WithCall_JSOps. There's two types of protos, XPC_WN_ModsAllowed_Proto_JSClass and XPC_WN_NoMods_Proto_JSClass, and for both of the types of protos we probably need two classes, one for each of the JSOps, as there's 4 different combinations going on here. Wrappers with call, that allow mods on the proto and don't, and wrappers w/o call, that allow mods on the proto and don't. So I think what this means is that we'll actually end up with two more JSClasses for the wrapper protos, so that we can expose the right ops in all cases.

I don't have this working yet, but I think it would work. And if we instrument this code a bit, we may find that the majority of the wrappers are of one type (which is probably the common case in the DOM code, as most if not all the DOM wrappers allow modifications to their prototype).

Comment 12

[Johnny Stenback (:jst)]

Attachment: Make XPConnect's protos share their wrappers JSObjectOps.

This patch does what I talked about above. It fixes all 4 combinations (there's actually 5, it turns out, the last one which will be dealt with in a different bug if at all). With this patch we share scopes with a wrappers proto in all cases where the wrapper has a prototype, which is the case for all DOM things. The remaining part is wrappers for natives that have no classinfo or scriptable helper or anything (i.e. plain XPCOM objects).

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 293041 [details] [diff] [review]
Make XPConnect's protos share their wrappers JSObjectOps.

>+    if (mScriptableInfo) {
>+        const XPCNativeScriptableFlags& flags(mScriptableInfo->GetFlags());
>+
>+        if (flags.AllowPropModsToPrototype())

jband-style if( and left brace style alert.

Looks good otherwise. r/sr/a=me.

/be

Comment 14

[Johnny Stenback (:jst)]

Fixed the style nits, and a couple more that I stumbled on. Filed bug 408301 on the remaining issue with wrappers w/o a proto. Marking FIXED.