Closed Bug 1391405 Opened 7 years ago Closed 7 years ago

Speed up schema normalization

Categories

(WebExtensions :: General, enhancement)

Product:
WebExtensions
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Performance Impact:high, firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.
59 bytes, text/x-review-board-request
gkrizsanits
: review+
qdot
: review+
Details
Bug 1391405: Part 2 - Speed up base type normalization.
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1391405: Part 3a - Speed up schema normalization for choices types.
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1391405: Part 3b - Speed up schema normalization for choices types some more.
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1391405: Part 4 - Avoid easily-avoidable regexp.
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1391405: Part 5 - Add helper for retrieving the enumerable value properties of a cross-compartment object.
59 bytes, text/x-review-board-request
gkrizsanits
: review+
qdot
: review+
Details
Bug 1391405: Part 6 - Use native helper for extracting enumerable properties.
59 bytes, text/x-review-board-request
zombie
: review+
Details 
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.
Blocks: 1385618
Whiteboard: [qf]
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.
Attachment #8898497 - Flags: review?(bobbyholley) → review-
Comment on attachment 8898498 [details]
Bug 1391405: Part 2 - Speed up base type normalization.

https://reviewboard.mozilla.org/r/169858/#review175352
Attachment #8898498 - Flags: review?(tomica) → review+
Comment on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

Sorry, I'm swamped.
Attachment #8898497 - Flags: review?(bobbyholley) → review?(gkrizsanits)
Attachment #8898502 - Flags: review?(bobbyholley) → review?(gkrizsanits)
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... ;)
Attachment #8898499 - Flags: review?(tomica) → review+
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 ?
Attachment #8898500 - Flags: review?(tomica) → review+
Comment on attachment 8898501 [details]
Bug 1391405: Part 4 - Avoid easily-avoidable regexp.

https://reviewboard.mozilla.org/r/169864/#review175440
Attachment #8898501 - Flags: review?(tomica) → review+
Comment on attachment 8898503 [details]
Bug 1391405: Part 6 - Use native helper for extracting enumerable properties.

https://reviewboard.mozilla.org/r/169868/#review175556
Attachment #8898503 - Flags: review?(tomica) → review+
Whiteboard: [qf] → [qf:p1]
Why don't you use cloneInto or something similar? Is structure cloning too slow, or you deliberately want to avoid some of its behavior?
Flags: needinfo?(kmaglione+bmo)
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?
Attachment #8898497 - Flags: review?(gkrizsanits)
(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.
Flags: needinfo?(kmaglione+bmo)
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.
Attachment #8898502 - Flags: review?(gkrizsanits) → review?(bobbyholley)
Attachment #8898502 - Flags: review?(bobbyholley) → review?(gkrizsanits)
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.
Attachment #8898502 - Flags: review?(gkrizsanits) → review+
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.
Attachment #8898497 - Flags: review+
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 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 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.
(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 on attachment 8898497 [details]
Bug 1391405: Part 1 - Add WebIDL versions of much used Components.utils helpers.

https://reviewboard.mozilla.org/r/169856/#review177626
Attachment #8898497 - Flags: review+
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
Attachment #8898502 - Flags: review+
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
Product: Toolkit → WebExtensions
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: