1326105 - Don't capture an async stack for a callback that no one cares about

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

When looking at the profiles for bug 1285874, I noticed a bunch of time taken capturing async stacks for removeEventListener calls.  This is happening because we create an EventListener callback object during the call, which captures the async stack.

The thing is, no one holds on to this EventListener, so there is no reason to care about its async stack.

We have an existing optimization to avoid calling HoldJSObjects() in this case; I'm going to expand it to skip capturing the async stack.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Performance testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. dom::RootedCallback should hold on to a JSContext that it can then use in its destructor

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Rename CallbackObject::HoldJSObjectsIfMoreThanOneOwner to a more generic name and hand it a JSContext to use

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Move async stack capture out of the 'fast' CallbackObject constructor and into FinishSlowJSInitIfMoreThanOneOwner

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove the now-unused JSContext argument of the protected CallbackObject constructor

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Move the getting of the incumbent global to the finish-slow-js codepath too, since it's not needed if no one will ever call our callback

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove the now-unused JSContext argument of the protected CallbackObject constructor

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Move the getting of the incumbent global to the finish-slow-js codepath too, since it's not needed if no one will ever call our callback

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822272 [details] [diff] [review]
part 3.  Move async stack capture out of the 'fast' CallbackObject constructor and into FinishSlowJSInitIfMoreThanOneOwner

I still wonder how useful async stack actually is.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822270 [details] [diff] [review]
part 1.  dom::RootedCallback should hold on to a JSContext that it can then use in its destructor

Please add a comment why raw member variable is a safe thing, especially in a class which isn't explicitly marked to be stack-only.
Could we make RootedCallback MOZ_STACK_CLASS ? or even MOZ_RAII?
Though, looks like JS::Rooted is already MOZ_RAII. So I guess RootedCallback inherits that annotation, but it just isn't visible to the reader.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822339 [details] [diff] [review]
part 5.  Move the getting of the incumbent global to the finish-slow-js codepath too, since it's not needed if no one will ever call our callback

Does this end up helping with bug 1273481 some more?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Could we make RootedCallback MOZ_STACK_CLASS ? or even MOZ_RAII?

I added an explicit MOZ_RAII here.

> Does this end up helping with bug 1273481 some more?

Yes, it does.  Times on the microbenchmark there are like so, over here in a nightly clean profile (so with async stacks):

Current tip: ~1450
With parts 1-4: ~570
With parts 1-5: ~360

For comparison, Safari is at ~230 and Chrome at ~1100.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/946d799f43af
part 1.  dom::RootedCallback should hold on to a JSContext that it can then use in its destructor.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd5b7e039015
part 2.  Rename CallbackObject::HoldJSObjectsIfMoreThanOneOwner to a more generic name and hand it a JSContext to use.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/56abaeb56804
part 3.  Move async stack capture out of the 'fast' CallbackObject constructor and into FinishSlowJSInitIfMoreThanOneOwner.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/413acc1afbd6
part 4.  Remove the now-unused JSContext argument of the protected CallbackObject constructor.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fdb9ee08a30
part 5.  Move the getting of the incumbent global to the finish-slow-js codepath too, since it's not needed if no one will ever call our callback.  r=smaug

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/946d799f43af
https://hg.mozilla.org/mozilla-central/rev/dd5b7e039015
https://hg.mozilla.org/mozilla-central/rev/56abaeb56804
https://hg.mozilla.org/mozilla-central/rev/413acc1afbd6
https://hg.mozilla.org/mozilla-central/rev/9fdb9ee08a30