Can we cache the proto for DOM Xrays?

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
> 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.
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... :(
MozReview-Commit-ID: 5IRrE8EmL9A
Attachment #8850284 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: ID9vMG3iJfZ
Attachment #8850285 - Flags: review?(bobbyholley)
(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.
Attachment #8850284 - Flags: review?(bobbyholley) → review+
Attachment #8850285 - Flags: review?(bobbyholley) → review+
Attachment #8850287 - Flags: review?(bobbyholley) → review+
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

2 years ago
Pushed by bzbarsky@mozilla.com:
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
That bustage was from bug 1349690.
Flags: needinfo?(bzbarsky)

Comment 13

2 years ago
Pushed by bzbarsky@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53b6c8e23941
https://hg.mozilla.org/mozilla-central/rev/81aa15f00924
https://hg.mozilla.org/mozilla-central/rev/26b0c85cce69
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.