Speed up schema normalization

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(7 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1385618
(Assignee)

Updated

2 years ago
Whiteboard: [qf]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-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/#review175158

These should go on ChromeUtils. ThreadSafeChromeUtils is exposed to workers, where using XPConnect will crash.
Attachment #8898497 - Flags: review?(bobbyholley) → review-

Comment 9

2 years ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1362226

Comment 19

2 years ago
mozreview-review
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 20

2 years ago
mozreview-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 21

2 years ago
mozreview-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 22

2 years ago
mozreview-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 24

2 years ago
mozreview-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/#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)
(Assignee)

Comment 25

2 years ago
(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)
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
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.
(Assignee)

Updated

2 years ago
Attachment #8898502 - Flags: review?(gkrizsanits) → review?(bobbyholley)
(Assignee)

Updated

2 years ago
Attachment #8898502 - Flags: review?(bobbyholley) → review?(gkrizsanits)

Comment 27

2 years ago
mozreview-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/#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 28

2 years ago
mozreview-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?
(Assignee)

Comment 30

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 31

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 32

2 years ago
(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

2 years ago
mozreview-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/#review177626
Attachment #8898497 - Flags: review+

Comment 34

2 years ago
mozreview-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+
(Assignee)

Comment 35

2 years ago
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

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.