1348095 - Can we cache the proto for DOM Xrays?

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

I was playing with the testcase in bug 613498 a bit, and the profile shows a quarter of the time under Xray access to DOM objects being XrayWrapper<CrossCompartmentWrapper, DOMXrayTraits>::getPrototype.

This is not surprising; for a .nextSibling access on an element we have to do at least 4 getPrototype calls (element-specific proto, HTMLElement.prototype, Element.prototype, Node.prototype) before we find the property.  And each call is not that cheap, not least because it has to JS_WrapObject.

But the proto of a DOM Xray should be immutable, right?  Could we just store it in a slot after we get it the first time, perhaps?

Comment 1

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #0)
> But the proto of a DOM Xray should be immutable, right?

Nope, see the JSSLOT_EXPANDO_PROTOTYPE stuff at http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/js/xpconnect/wrappers/XrayWrapper.cpp#2413

> Could we just store
> it in a slot after we get it the first time, perhaps?

So, we already have the slot on the expando object, and could cache it there (assuming the Xray hook always returns the same result), at the cost of potentially creating a lot more expando objects. We could also separately cache the native prototype on the holder, at the expense of having overlapping slots between the holder and expando objects.

At first glance, the latter approach is probably preferable, but I don't know how many expando objects and holder objects get instantiated in practice.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> At first glance, the latter approach is probably preferable

Yes, I agree.  We'd still have annoying wrapping overhead on getPrototype from the expando object, since it's stored in the target compartment. But if one sets __proto__ on an Xray _and_ expects good performance, all I can offer is a nice padded room.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, the obvious reason to cache on the holder, not the expando, is that then it's actually in the right compartment, so the nasty WrapObject overhead is not present.

Testcase, to be run in a chrome scratchpad targeting a non-e10s window:

(function() {
  var node = content.document.documentElement;
  var start = performance.now()
  var count = 100000;
  for (var i = 0; i < count; ++i) {
    node.nextSibling;
  }
  var stop = performance.now();
  alert(Math.round((stop - start)/count * 1e6));
})();

Without this bug fixed I see numbers in the 2800 range.  With it fixed, I see numbers closer to 2100.

Without the IEFE, all the numbers are larger by 1000 or so.  Presumably, writing, and maybe reading, global variables in the sandbox that scratchpad uses is horribly slow... :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the unused reserved slots from the XPCWN xray holder

MozReview-Commit-ID: 5IRrE8EmL9A

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Give all the Xray holder's a JSClass that has a slot for caching a prototype

MozReview-Commit-ID: ID9vMG3iJfZ

Comment 6

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3)
> Without the IEFE, all the numbers are larger by 1000 or so.  Presumably,
> writing, and maybe reading, global variables in the sandbox that scratchpad
> uses is horribly slow... :(

I suspect that has something to do with Scratchpad compiling the code to return a value. As I understand it, that disables most JIT optimizations at the top-level.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Cache the proto of an Xray on its holder, so we don't have to keep re-wrapping it

MozReview-Commit-ID: I78AoSB3TNW

Comment 8

[Boris Zbarsky [:bzbarsky]]

Try found a bug: the holders for OpaqueXrayTraits didn't have the slot, because I didn't find its createHolder method (it was in the header).

Fixed that by moving the new JSClass up to XrayTraits like so:

  const JSClass XrayTraits::HolderClass = {
    "XrayHolder", JSCLASS_HAS_RESERVED_SLOTS(HOLDER_SHARED_SLOT_COUNT)
  };

and pushing to try again.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60471d1feaa4
part 1.  Remove the unused reserved slots from the XPCWN xray holder.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/3efe3c6f4e7f
part 2.  Give all the Xray holders a JSClass that has a slot for caching a prototype.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba41a1bd062
part 3.  Cache the proto of an Xray on its holder, so we don't have to keep re-wrapping it.  r=bholley

Comment 10

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1e0598886d349e919fbe71ab3534574e13d40468&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception because of bustage https://treeherder.mozilla.org/logviewer.html#?job_id=85840526&repo=mozilla-inbound

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42b182f3367e
Backed out changeset 8ba41a1bd062 
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf77bf80ba2
Backed out changeset 3efe3c6f4e7f

Comment 12

[Boris Zbarsky [:bzbarsky]]

That bustage was from bug 1349690.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b6c8e23941
part 1.  Remove the unused reserved slots from the XPCWN xray holder.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/81aa15f00924
part 2.  Give all the Xray holders a JSClass that has a slot for caching a prototype.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/26b0c85cce69
part 3.  Cache the proto of an Xray on its holder, so we don't have to keep re-wrapping it.  r=bholley

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/53b6c8e23941
https://hg.mozilla.org/mozilla-central/rev/81aa15f00924
https://hg.mozilla.org/mozilla-central/rev/26b0c85cce69