Open Bug 703047 Opened 9 years ago Updated 5 years ago

(ObjShrink) Avoid shape changes on XPConnect wrapper prototype changes

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

XPConnect uses SetProto a lot on certain classes wrapping native objects, primarily XPC_WN_ModsAllowed_NoCall_Proto_JSClass.  Since shape is required to determine prototype, changing the prototype requires a shape change, and since the prototype is not itself encoded in the shape this shape change requires conversion to dictionary mode.  This causes a substantial number of dictionary conversions on some Dromaeo DOM tests.

The simple way to fix this is to just not cache prototype lookups across such objects, so that a shape change is not required when their prototype is mutated.  The better fix is I think to remove the constraint that shape determines prototype entirely, along with the shape teleporting optimization.  Neither of these does all that much given TI, we should be able to resolve prototype lookups statically.

To better understand the performance implications here, I'll do the simple fix for now and test removal of these invariants after ObjShrink merges to trunk.
Attached patch patch (obsolete) — Splinter Review
https://hg.mozilla.org/projects/jaegermonkey/rev/b6485471d6f7
Assignee: general → bhackett1024
Attachment #574973 - Flags: review?(luke)
Hmm...  Why does xpconnect have to SetProto here, exactly?
Also, which exact objects end up going through the JS_NewObjectWithUncacheableProto codepath?
Some of this is for installing and uninstalling global name polluters, but there are other uses of SetProto too which I haven't looked into.

The only user of NewObjectWithUncacheableProto is xpc_NewSystemInheritingJSObject, whose purpose I don't know.  It gets used to make various xpconnect objects, and seems to be the only method by which XPC_WN_ModsAllowed_NoCall_Proto_JSClass objects get created.

The assumption I'm working with here is that the only xpconnect-related prototype lookups we can reasonably expect or want to be cacheable are on DOM nodes themselves, where getter hooks and methods are stored on the prototype.  I think these objects are created somewhere other than xpc_NewSystemInheritingJSObject.
Comment on attachment 574973 [details] [diff] [review]
patch

Gross.  Could you file a followup for measuring and hopefully removing teleporting?
Attachment #574973 - Flags: review?(luke) → review+
Brian, I'd really like Bobby to actually verify those assumptions.  I do know that we care about prototype lookups on all DOM-exposed objects, not just DOM nodes.  For example, performance of lookups on CSSStyleDeclaration objects is pretty important.
Filed bug 703164 for weakening shape invariants and removing teleporting.
Depends on: 703164
(In reply to Boris Zbarsky (:bz) from comment #6)
> Brian, I'd really like Bobby to actually verify those assumptions.  I do
> know that we care about prototype lookups on all DOM-exposed objects, not
> just DOM nodes.  For example, performance of lookups on CSSStyleDeclaration
> objects is pretty important.

I unfortunately don't this stuff well enough yet to say with certainty which cases matter for performance.

CCing mrbkap who hopefully knows the answer to the offhand.
This patch hurt Dromaeo (though helped the number of created shapes a lot), so some of the uses of this function do matter for performance --- some DOM elements are created on this path.  Yesterday I looked at this some more, and tried an alternative which only inhibits prototype caching for objects which have had a dynamic SetProto (this is more robust and JS internal, and allows removing the new API).  That fixed the regression (vs. previous JM revisions; there is still a ~4% Dromaeo regression on JM vs. trunk), I'll attach a patch here later today.
Attached patch round 2Splinter Review
Attachment #574973 - Attachment is obsolete: true
Attachment #575540 - Flags: review?(luke)
Attached patch round 3Splinter Review
Loosen things so that an object's prototype is only marked as uncacheable once its prototype has been overwritten twice.  The DOM has some behavior I don't understand at all where the prototype of certain objects can change once (and only once) after creation (ResolvePrototype in dom/base/nsDOMClassInfo.cpp), and some hot lookups in MooTools end up going through this prototype link.

This fixes a large MooTools regression on the ObjShrink branch.  I don't feel so bad about making these fragile fixes; mutation of prototypes after creation is not something which websites do, but is rather a peculiarity of our DOM code which the JS engine needs to be bent to accommodate.  It would be nice to remove some of the prototype manipulation from the DOM and XPConnect, but I don't know how hard this would be to do.
Attachment #576988 - Flags: review?(luke)
So is the "peculiarity of our DOM code" a bug itself?  If the logic behind the code doesn't make good sense, why is it the way it is and should it be changed?
(In reply to Paul Wright from comment #14)
> So is the "peculiarity of our DOM code" a bug itself?  If the logic behind
> the code doesn't make good sense, why is it the way it is and should it be
> changed?

I don't think this is necessarily a bug, it may be that restructuring the code to avoid the prototype mutations would make things slower or more complicated.  It is definitely the case though that mutating prototypes (or parents) is a significant negative for performance if not done carefully.
Peter should be able to answer any DOM prototype setup questions!
Depends on: 705859
Depends on: 705895
Attachment #576988 - Flags: review?(luke) → review+
Attachment #575540 - Flags: review?(luke) → review+
Depends on: 712379
Depends on: 741212
You need to log in before you can comment on or make changes to this bug.