Boris Zbarsky [:bzbarsky] Assignee
	Description • 11 years ago

This is a proposal for addressing use cases A and B from <https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682>.  In particular, it's a proposal for how to accomplish it without forcing C++ consumers (including js-implemented binding codegen) to manually create JS arrays and populate them.

The idea is to build on the infrastructure from bug 935855 as follows:

1) Introduce the concept of a "Cached" attribute.  This may be identical to what "StoreInSlot" does right now, or "StoreInSlot" may imply "Cached" but not vice versa.  See below for the tradeoffs.  "Cached" attributes would have a slot reserved for them in the JSObject.  Bikeshedding on the names welcome.

2) Make the IDL getter for a cached attribute check the slot for a non-undefined value before calling the C++ getter, and cache the return value in the slot.  This slightly generalizes the code from bug 935855, where we effectively do the get-and-cache thing only when the JS wrapper is first created.

3) When caching the return value in the slot, preserve the wrapper if the C++ object is wrappercached.

4) Allow sequence-valued attributes if they're marked as cached.

At this point, the C++ code would do its normal nsTArray thing when it's called, but after that it wouldn't get called again unless the slot is cleared.  The binding code would expose methods for doing that clearing to C++ for sure (e.g. we need that on Window).  If JS-implemented bindings need to do the clearing bit too, we'd add something for them, but if not I'd rather not have an extra ChromeOnly footgun around.

Some open questions:

Should "Cached" be identical to "StoreInSlot"?  The pro is that it's simpler (one extended attribute, less different code).  The cons are twofold.  First, it forces population of the cache at wrapper-creation time, because StoreInSlot guarantees to the JIT that the slot is populated.  I expect this is not great for contacts.  Second, this places a limit on the number of cached attributes an object can have: it's limited by the number of fixed slots the JS engine supports (I'm told 16, but maybe it's 31 now).  Not sure whether this will be an issue in practice.  Either way, simply separating the two concepts, and having "StoreInSlot" imply fixed slot and the JIT reading it directly, while "Cached" simply implies a reserved (but possibly dynamic) slot and no direct JIT gets, would address both cons...

Reuben, thoughts on this from the pov of Contacts?  In particular, on the eager vs lazy generation of the arrays.

	Reuben Morais [:reuben]
	Comment 1 • 11 years ago

A few data points to consider: we have 18 array properties in the mozContact interface, 12 of which are arrays of DOMString. For the use cases where performance matters the most for us, I'd say 6 are hot, 4 being arrays of DOMString, one being array of another Contacts interface (ContactTelField), and one being array of Blob.

When we converted Contacts to WebIDL, the creation of wrappers popped up in profiles. We worked around it by creating them lazily when the property is read, so I think you're right that eagerly creating all array properties is not good for Contacts.

And IIUC, Contacts does not need to be able to clear the slot.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 11 years ago

OK.  I think that settles it in favor of two separate annotations, one of which promises a fixes slot with eager init and one of which does not.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 11 years ago

Comment on attachment 8338033 [details] [diff] [review]
part 2.  Make getters for cached properties check and set the reserved slot as needed.

Er, wait.  Compartment fail on my part in part 2...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 11 years ago

I'm not sure that CGClearCachedValueMethod.definition_body is much better than just having two different branches both spitting out code...  That might have been more readable.  Let me know if you'd prefer that?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 11 years ago

MaybeWrapValue is super-slow for non-DOM objects (always ends up calling JS_WrapValue on the off chance they're an XPConnect object), so avoid it.  But maybe we should fix it to check for XPConnect objects?

	Peter Van der Beken [:peterv]
	Comment 13 • 11 years ago

Comment on attachment 8338042 [details] [diff] [review]
part 2.  Make getters for cached properties check and set the reserved slot as needed.

Review of attachment 8338042 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +5060,5 @@
> +                "// Be careful here: Have to wrap the value into the\n"
> +                "// compartment of realDOMObj before storing, since we might\n"
> +                "// be coming in via Xrays and the value is already in the\n"
> +                "// caller compartment.\n"
> +                "{ // Scope for temp value\n"

maybe "for tempVal"?

@@ +5925,5 @@
> +                "  JS::Rooted<JSObject*> realDOMObj(cx);\n"
> +                "  // Safe to do an unchecked unwrap, since we've gotten this far.\n"
> +                "  // Also make sure to unwrap outer windows, since we want the\n"
> +                "  // real DOM object.\n"
> +                "  realDOMObj = IsDOMObject(obj) ? obj : js::UncheckedUnwrap(obj, /* stopAtOuter = */ false);\n"

Should we just call this reflector?

@@ +5931,5 @@
> +                "    // Scope for cachedVal\n"
> +                "    JS::Value cachedVal = js::GetReservedSlot(realDOMObj, %d);\n"
> +                "    if (!cachedVal.isUndefined()) {\n"
> +                "      args.rval().set(cachedVal);\n"
> +                "      // Who knows what compartment the cached value is in?\n"

Don't we want the cached value to live in the reflector's compartment? It certainly looks like that's what you do above?

	Peter Van der Beken [:peterv]
	Comment 14 • 11 years ago

Comment on attachment 8338048 [details] [diff] [review]
part 3.  Preserve the wrapper when caching things in its slots if we're an nsWrapperCache.

Review of attachment 8338048 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +5058,5 @@
>          if self.idlNode.isAttr() and self.idlNode.slotIndex is not None:
> +            # For the case of Cached (but NOT StoreInSlot, since those get dealt
> +            # with during wrapper setup, and failure would involve us trying to
> +            # clear an already-preserved wrapper) attributes, go ahead and
> +            # preserve our wrapper if needed.

I'd rewrite a bit:

          # For the case of Cached attributes, go ahead and preserve our wrapper
          # if needed. We don't do this for StoreInSlot, those get dealt with
          # during wrapper setup, and failure would involve us trying to clear
          # an already-preserved wrapper.

Maybe also document why we need to preserve here? We just do this for performance reasons, right?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 11 years ago

> Should we just call this reflector?

I guess we can, yeah.

> We just do this for performance reasons, right?

No, we do it to ensure correctness.  The use case is having a C++ getter that always returns a new object but having the JS-facing API always return the same object.  The only way to get that behavior while storingthe JS-facing value in the JS object is to preserve the JSObject.

> Don't we want the cached value to live in the reflector's compartment?

Hmm.  We sort of do, but afaict ReparentWrapper doesn't do anything useful with reserved slots, right?

Is that just a bug in ReparentWrapper that we should fix (i.e. have it JS_WrapValue all the slots or something, or at least all the slots after the "normal" DOM ones)?  Bobby?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 11 years ago

Though I guess that just means the new wrapper will have no slots set, which means cached values will be lost on reparent.

In practice this may not matter for now because we're not caching anything on nodes yet.  But still seems like we may want to fix ReparentWrapper here somehow...

	Bobby Holley (:bholley)
	Comment 17 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #15)
> Is that just a bug in ReparentWrapper that we should fix (i.e. have it
> JS_WrapValue all the slots or something, or at least all the slots after the
> "normal" DOM ones)?  Bobby?

I'd be pretty nervous about blindly copying reserved slots in general. If we want to copy them, we should do it explicitly so that we know what we're copying and so that MXR will reveal the dep on the enum value for that slot.

I think it's also probably fine to lose cached values on reparent, even for nodes. But other folks may disagree.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 11 years ago

Talked to Bobby.  We're going to ignore the issue for now and assume that clearing cached values on reparent is in fact the right thing to do.  If that ever changes, we'll revisit.

	Peter Van der Beken [:peterv]
	Comment 20 • 11 years ago

Comment on attachment 8338339 [details] [diff] [review]
Part 5, but outputting the clearing methods for non-concrete interfaces too.

Review of attachment 8338339 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +2514,5 @@
> +            regetMember = ("\n"
> +                           "JS::Rooted<JS::Value> temp(aCx);\n"
> +                           "JSJitGetterCallArgs args(&temp);\n"
> +                           "if (!get_%s(aCx, obj, aObject, args)) {\n"
> +                           "  JS_ClearPendingException(aCx); // Or report?\n"

The getter most likely reported? When would we end up here with a pending exception? If we do, it seems like we should report, if we don't then we wouldn't need the clear :-).

@@ +2532,5 @@
> +                "if (!obj) {\n"
> +                "  return%s;\n"
> +                "}\n"
> +                "%s"
> +                "js::SetReservedSlot(obj, %d, JS::UndefinedValue());"

Given that the non-StoreInSlot case doesn't change between members apart from the wrapper and the index, we might want to share a non-generated implementation for those (taking a JSObject* and an index)?

	Peter Van der Beken [:peterv]
	Comment 21 • 11 years ago

Comment on attachment 8338218 [details] [diff] [review]
part 6.  Fix performance regression in the imagedata getter due to using MaybeWrapValue on a non-DOM object.

Review of attachment 8338218 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +4159,5 @@
> +    if type.isCallback() or type.isCallbackInterface() or type.isObject():
> +        if type.nullable():
> +            return "MaybeWrapObjectOrNullValue"
> +        else:
> +            return "MaybeWrapObjectValue"

No else after return.

@@ +4164,5 @@
> +    # Spidermonkey interfaces are never DOM objects
> +    if type.isSpiderMonkeyInterface():
> +        if type.nullable():
> +            return "MaybeWrapNonDOMObjectOrNullValue"
> +        else:

Ditto.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 11 years ago

> The getter most likely reported?

Certainly not; else pages could not catch exceptions from getters.

You're right that we should bite the bullet and just report here; I'll fix that.

> we might want to share a non-generated implementation for those

As discussed on IRC, not worth it: the shared bit would be one line.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 11 years ago

And for more future reference, a perf comparison for that hackypatch: http://dromaeo.com/?id=210890,210892

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 11 years ago

   https://hg.mozilla.org/integration/mozilla-inbound/rev/07fa802b9501
   https://hg.mozilla.org/integration/mozilla-inbound/rev/795879f6f204
   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b2e2bea10b
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a7878aac67f0
   https://hg.mozilla.org/integration/mozilla-inbound/rev/fad25ed7cdef
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f96a77402a26
   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8aa9f83968

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/07fa802b9501
https://hg.mozilla.org/mozilla-central/rev/795879f6f204
https://hg.mozilla.org/mozilla-central/rev/b9b2e2bea10b
https://hg.mozilla.org/mozilla-central/rev/a7878aac67f0
https://hg.mozilla.org/mozilla-central/rev/fad25ed7cdef
https://hg.mozilla.org/mozilla-central/rev/f96a77402a26
https://hg.mozilla.org/mozilla-central/rev/cb8aa9f83968

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 11 years ago

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached