Closed Bug 1401919 Opened 3 years ago Closed 3 years ago

Functions used after the seekToNextFrame promise are interpreted half the time

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file)

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.
Flags: needinfo?(bhackett1024)
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.
Flags: needinfo?(bhackett1024)
Attached patch PatchSplinter Review
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).
Attachment #8913465 - Flags: review?(jwwang)
Assignee: nobody → florian
Status: NEW → ASSIGNED
See Also: → 1401757
Comment on attachment 8913465 [details] [diff] [review]
Patch

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

A JS peer should review this code.
Attachment #8913465 - Flags: review?(jwwang) → review?(jorendorff)
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.
Attachment #8913465 - Flags: review?(jorendorff) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03487f70562a
Functions used after the seekToNextFrame promise should not be interpreted half the time, r=nbp.
https://hg.mozilla.org/mozilla-central/rev/03487f70562a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.