1401919 - Functions used after the seekToNextFrame promise are interpreted half the time

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Florian Quèze [:florian]]

See this profile: https://perfht.ml/2hm81On

The code being profiled is: https://pastebin.mozilla.org/9067853
To run it without hitting bug 1401757, the "media.dormant-on-pause-timeout-ms" pref needs to be set to -1 in about:config.

The onFrame function is called 115 times here. Half the time through Interpret and half the time through js::jit::EnterBaselineMethod. Each time the GC is called, the next run goes through Interpret.

nbp tells me there's an optimization to keep alive the functions used with requestAnimationFrame and thinks it may be possible to do the same thing for functions following the seekToNextFrame promise.

Comment 1

[Brian Hackett [Laid off!]]

Yes, the NotifyAnimationActivity() API is used to notify the JS engine of animation activity in a compartment, so that if it GCs while an animation is playing it will avoid discarding Ion/Baseline code in that compartment.

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch

This is a copy of the code at http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/dom/html/HTMLCanvasElement.cpp#1095

I verified with the profiler that this patch has the impact I expected (ie. once my function used with seekToNextFrame is baseline compiled, it doesn't go through the interpreter again after each GC).

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8913465 [details] [diff] [review]
Patch

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

A JS peer should review this code.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8913465 [details] [diff] [review]
Patch

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

I am no expert in DOM media element, but concerning the usage of the JSAPI, this patch sounds good to me.
NotifyAnimationActivity does not GC, so there is no reason for rooting the object.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03487f70562a
Functions used after the seekToNextFrame promise should not be interpreted half the time, r=nbp.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/03487f70562a