1391405 - Speed up schema normalization

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement)

Description

[Kris Maglione [:kmag]]

There are a lot of bottlenecks in our schema normalization code, a lot of them involving X-rays. We should be able to reduce the overhead a fair bit.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

In the code that I'm profiling, the XPC WrappedNative overhead of calling
these functions adds up to about a quarter of the time spent executing the
code. The overhead of the WebIDL versions is negligible.

Review commit: https://reviewboard.mozilla.org/r/169856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169856/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1391405: Part 2 - Speed up base type normalization.

The Array and ArrayBuffer type checks we do in getBaseType add up to a
significant amount of overhead given the number of times we call them,
especially when X-ray overhead comes into play. These changes allow us to
avoid X-ray overhead altogether.

Review commit: https://reviewboard.mozilla.org/r/169858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169858/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1391405: Part 3a - Speed up schema normalization for choices types.

For choices types, when one choice fails, we don't need the original error
string, since another choice may succeed, and we generate the final error
based on all of the options. Nevertheless, we spend a lot of time generating
JSON strings for the failed inputs in those cases, which adds up to about 12%
of the remaining overhead at this point.

Review commit: https://reviewboard.mozilla.org/r/169860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169860/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1391405: Part 3b - Speed up schema normalization for choices types some more.

Like part a, but for `choices` messages rather than error messages.

Review commit: https://reviewboard.mozilla.org/r/169862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169862/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1391405: Part 4 - Avoid easily-avoidable regexp.

Review commit: https://reviewboard.mozilla.org/r/169864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169864/

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1391405: Part 5 - Add helper for retrieving the enumerable value properties of a cross-compartment object.

As part of the normalization process for WebExtension API calls, we need to
extract and validate the full set of value properties (including properties
X-rays would normally deny access to) from cross-compartment objects. This
currently involves waiving X-rays, enumerating property descriptors, and
unwaiving X-rays - all through X-ray wrappers and waivers - and generating a
lot of expensive and short-lived wrappers in-between.

This helper reads out the list of safe properties from within the object's
compartment, and then copies them over to an object in the target compartment,
without any X-ray overhead, or any unnecessary intermediate wrappers or
compartment switches. It cuts about 40% off the overhead of our normalization
code.

Review commit: https://reviewboard.mozilla.org/r/169866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169866/

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1391405: Part 6 - Use native helper for extracting enumerable properties.

Review commit: https://reviewboard.mozilla.org/r/169868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169868/

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

https://reviewboard.mozilla.org/r/169856/#review175158

These should go on ChromeUtils. ThreadSafeChromeUtils is exposed to workers, where using XPConnect will crash.

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8898498 [details]
Bug 1391405: Part 2 - Speed up base type normalization.

https://reviewboard.mozilla.org/r/169858/#review175352

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169856/diff/1-2/

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8898498 [details]
Bug 1391405: Part 2 - Speed up base type normalization.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169858/diff/1-2/

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8898499 [details]
Bug 1391405: Part 3a - Speed up schema normalization for choices types.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169860/diff/1-2/

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8898500 [details]
Bug 1391405: Part 3b - Speed up schema normalization for choices types some more.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169862/diff/1-2/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8898501 [details]
Bug 1391405: Part 4 - Avoid easily-avoidable regexp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169864/diff/1-2/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8898502 [details]
Bug 1391405: Part 5 - Add helper for retrieving the enumerable value properties of a cross-compartment object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169866/diff/1-2/

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8898503 [details]
Bug 1391405: Part 6 - Use native helper for extracting enumerable properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169868/diff/1-2/

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

Sorry, I'm swamped.

Comment 19

[Tomislav Jovanovic :zombie]

Comment on attachment 8898499 [details]
Bug 1391405: Part 3a - Speed up schema normalization for choices types.

https://reviewboard.mozilla.org/r/169860/#review175530

r+ (though this looks like it could be getters all the way down... ;)

Comment 20

[Tomislav Jovanovic :zombie]

Comment on attachment 8898500 [details]
Bug 1391405: Part 3b - Speed up schema normalization for choices types some more.

https://reviewboard.mozilla.org/r/169862/#review175546

::: toolkit/components/extensions/Schemas.jsm:508
(Diff revision 2)
>  
> -      choices = Array.from(choices);
> -      if (choices.length == 1) {
> -        currentChoices.add(choices[0]);
> +      if (choices.size == 1) {
> +        for (let choice of choices) {
> +          currentChoices.add(choice);
> +        }
>        } else if (choices.length) {

choices.size ?

Comment 21

[Tomislav Jovanovic :zombie]

Comment on attachment 8898501 [details]
Bug 1391405: Part 4 - Avoid easily-avoidable regexp.

https://reviewboard.mozilla.org/r/169864/#review175440

Comment 22

[Tomislav Jovanovic :zombie]

Comment on attachment 8898503 [details]
Bug 1391405: Part 6 - Use native helper for extracting enumerable properties.

https://reviewboard.mozilla.org/r/169868/#review175556

Comment 23

[Gabor Krizsanits (INACTIVE)]

Why don't you use cloneInto or something similar? Is structure cloning too slow, or you deliberately want to avoid some of its behavior?

Comment 24

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

https://reviewboard.mozilla.org/r/169856/#review176246

::: dom/base/ChromeUtils.cpp:157
(Diff revision 2)
> +  if (!aVal.isObject()) {
> +      aRetval.set(aVal);
> +      return;
> +  }
> +
> +  JS::RootedObject obj(aGlobal.Context(), js::UncheckedUnwrap(&aVal.toObject()));

Why do you need UncheckedUnwrap here and below? If this is a chrome API (used by one) we should never encounter a security wrapper here no?

Comment 25

[Kris Maglione [:kmag]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> Why don't you use cloneInto or something similar? Is structure cloning too
> slow, or you deliberately want to avoid some of its behavior?

It's complicated, but essentially we need a shallow clone. There are places where we need the original objects rather than wrappers, or we need to access non-clonable objects, and there are other places where we need to clone the arguments for cross-process messaging, anyway, and avoiding the extra clone is important for performance reasons.

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

https://reviewboard.mozilla.org/r/169856/#review176246

> Why do you need UncheckedUnwrap here and below? If this is a chrome API (used by one) we should never encounter a security wrapper here no?

Well, for a start, this is copied almost verbatim from the Components.utils implementation.

But, basically, security wrappers aren't the issue here, any cross-compartment wrappers are. In this case, unwrapping is probably not necessary, since we should always unwrap before re-wrapping. But UncheckedUnwrap is faster than the CheckedUnwrap we'd use otherwise, so that may make a measurable difference.

For the one below, it's a question of which object we're getting the class name of. If we don't unwrap before checking the class name, it will pretty much always be 'Proxy' for cross-compartment objects, which is not what we want most of the time.

Comment 27

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8898502 [details]
Bug 1391405: Part 5 - Add helper for retrieving the enumerable value properties of a cross-compartment object.

https://reviewboard.mozilla.org/r/169866/#review177234

::: dom/base/ChromeUtils.cpp:203
(Diff revision 2)
> +      js::ReportAccessDenied(cx);
> +      return;
> +    }
> +    JSAutoCompartment ac(cx, obj);
> +
> +    if (!JS_Enumerate(cx, obj, &ids) ||

Let's make sure we're not dealing with a scripted proxy before all the JS calls.

Comment 28

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

https://reviewboard.mozilla.org/r/169856/#review177238

::: dom/base/ChromeUtils.cpp:157
(Diff revision 2)
> +  if (!aVal.isObject()) {
> +      aRetval.set(aVal);
> +      return;
> +  }
> +
> +  JS::RootedObject obj(aGlobal.Context(), js::UncheckedUnwrap(&aVal.toObject()));

I don't think CheckedUnwrap is measurably slower but it's not a huge risk here either to use the unchecked version since we either rewrap or just get a class name, just it looked a bit suspicious at first.

Comment 29

[Gabor Krizsanits (INACTIVE)]

The patches make sense, my only concern here is the testing part. On the web extension front this code is going to be well covered right? So for a vanilla JS object with a DOM object in it's proto chain, what is the expected result for class name?

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8898502 [details]
Bug 1391405: Part 5 - Add helper for retrieving the enumerable value properties of a cross-compartment object.

https://reviewboard.mozilla.org/r/169866/#review177234

> Let's make sure we're not dealing with a scripted proxy before all the JS calls.

We do check this on the JS side before we call this, but I guess it may make sense to check on the native side too.

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

https://reviewboard.mozilla.org/r/169856/#review177238

> I don't think CheckedUnwrap is measurably slower but it's not a huge risk here either to use the unchecked version since we either rewrap or just get a class name, just it looked a bit suspicious at first.

It probaly isn't measurably slower for chrome callers, but I'm not sure. Given that, with the number of times we call this, the XPConnect version is significantly slower than the WebIDL version, it may be enough to add up.

But there's another reason the UncheckedUnwrap is necessary, that I forgot about. The wrapper factory code looks at the object that we pass to JS_WrapObject to determine if we want X-ray waivers. And since we're explicitly trying to unwaive X-rays here, we need to explicitly unwrap the object first, or we'll wind up with the same X-ray waiver wrappers on the return object that we had on the input.

Comment 32

[Kris Maglione [:kmag]]

(In reply to Gabor Krizsanits [:krizsa :gabor] (PTO until 28th) from comment #29)
> The patches make sense, my only concern here is the testing part. On the web
> extension front this code is going to be well covered right?

Yes, we have tests for the cases that we care about.

> So for a vanilla JS object with a DOM object in it's proto chain, what is
> the expected result for class name?

That's a good question. As it currently stands, it would return "Object",
which is arguably what we expect. There might be some valid argument that
`Object.create(new ImageData())` should work as an ImageData object, and we
should return "ImageData" as the class name. But those objects don't work in
most of the places a native ImageData object would.

However, something like `getClassName(new class extends Map {})` does return
"Map". But since that object *does* work anywhere a Map would (including areas
like structured clone serialization), I think that also makes sense.

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

https://reviewboard.mozilla.org/r/169856/#review177626

Comment 34

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8898502 [details]
Bug 1391405: Part 5 - Add helper for retrieving the enumerable value properties of a cross-compartment object.

https://reviewboard.mozilla.org/r/169866/#review177628

Comment 35

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/114ff6330fcb2879d0092a10777a018a4ed2d495
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers. r=gabor,qdot

https://hg.mozilla.org/integration/mozilla-inbound/rev/51dc5499d4b388f88bcf10031e99980fdc6a81a7
Bug 1391405: Part 2 - Speed up base type normalization. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/adbdc80ee20752d83152b19c947b779d5fa2ef27
Bug 1391405: Part 3a - Speed up schema normalization for choices types. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/79db2bb9824300da5866a8e9a1c9a023cf40d1fa
Bug 1391405: Part 3b - Speed up schema normalization for choices types some more. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/41ce100cb4dabb65d0e944e56c3a19ec8f928af1
Bug 1391405: Part 4 - Avoid easily-avoidable regexp. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/c617d5572903568f4e861b8b648cb6aa9b15fada
Bug 1391405: Part 5 - Add helper for retrieving the enumerable value properties of a cross-compartment object. r=gabor,qdot

https://hg.mozilla.org/integration/mozilla-inbound/rev/80a0c38278154ddf519ee8ff9e7239e938540b37
Bug 1391405: Part 6 - Use native helper for extracting enumerable properties. r=zombie

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/114ff6330fcb
https://hg.mozilla.org/mozilla-central/rev/51dc5499d4b3
https://hg.mozilla.org/mozilla-central/rev/adbdc80ee207
https://hg.mozilla.org/mozilla-central/rev/79db2bb98243
https://hg.mozilla.org/mozilla-central/rev/41ce100cb4da
https://hg.mozilla.org/mozilla-central/rev/c617d5572903
https://hg.mozilla.org/mozilla-central/rev/80a0c3827815