1655683 - nsRFPService::ReduceTimePrecisionImpl is slow

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

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

https://share.firefox.dev/3hHxX4B

Comment 1

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

Dealing with timestamps should be very fast, but looks like ReduceTimePrecisionImpl ends up even doing allocations.

Comment 3

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

https://share.firefox.dev/34EK2EC

https://farre.github.io/web-tools/rIC/index.html is a testcase.
Ensure "Show total allotted remaining time per update" and
"Use requestAnimationFrame" and
"Respect time remaining" are checked
and "Time to spend in idle callback" is 10
and click Toggle

Comment 4

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

In that test it is expected that lots of time is spent in TimeRemaining, but looks like nsRFPService::ReduceTimePrecisionImpl is adding
quite a bit overhead there.
Is it RandomMidpoint calls which are slow? They are using a lock(!) in this super hot code path.

Comment 5

[Tom Ritter [:tjr]]

(In reply to Olli Pettay [:smaug] from comment #4)

In that test it is expected that lots of time is spent in TimeRemaining, but looks like nsRFPService::ReduceTimePrecisionImpl is adding
quite a bit overhead there.
Is it RandomMidpoint calls which are slow? They are using a lock(!) in this super hot code path.

I'm not very great at using the profiler but I would expect that - yes. https://bugzilla.mozilla.org/show_bug.cgi?id=1655777#c9 has some suggestions for narrowing down what part of the code the issue is in.

We could probably just remove the lock. We might hit race conditions: the impact of doing so would mean that individual timestamps might be jittered using the incorrect random values. But it would be very difficult for an attacker to control this, determine if it happened, or actually exploit this in a meaningful way. We could also look at the possibility of removing jitter entirely....

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to Olli Pettay [:smaug] from comment #3)

https://share.firefox.dev/34EK2EC

https://farre.github.io/web-tools/rIC/index.html is a testcase.
Ensure "Show total allotted remaining time per update" and
"Use requestAnimationFrame" and
"Respect time remaining" are checked
and "Time to spend in idle callback" is 10
and click Toggle

I'm not so sure if I read the profiler correctly, but from this, I found there are some mozilla::detail::MutexImpl::lock above some nsRFPService::ReduceTimePrecisionImpl calls. And the only place which uses mutex lock in ReduceTimePrecision is RandomMidpoint.

Comment 7

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

https://bugzilla.mozilla.org/show_bug.cgi?id=1686930 is another instance, there this shows up really badly.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

is this resolved now?

Comment 9

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

I profiled https://farre.github.io/web-tools/rIC/index.html again and it looked fine.

Comment 10

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

(hmm, why the needinfo wasn't cleared in comment 9)