1813310 - AutoProfileRunnable uses TimeStamp::Now even when the profiler isn't running

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

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

https://searchfox.org/mozilla-central/rev/4d2b1f753871ce514f9dccfc5b1b5e867f499229/tools/profiler/public/ProfilerRunnable.h#17,24,32,50

This shows up in speedometer, especially on linux where ::Now() is slow
https://share.firefox.dev/3kOggYb VanillaJS-TodoMVC

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1688300

:florian, since you are the author of the regressor, bug 1688300, could you take a look?

For more information, please visit auto_nag documentation.

Comment 2

[Julien Wajsberg [:julienw]]

Should we check at [1] if profiler_thread_is_being_profiled_for_markers() before adding this RAII? My understanding is that most Runnable are quick so it would be better to check it here rather than in the constructor and destructor. Thoughts?

[1] https://searchfox.org/mozilla-central/rev/7a9e3bbab8f81c2cbc72a394047f948da9cfef9a/tools/profiler/public/ProfilerRunnable.h#17

Comment 3

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

The particular case in the profile is even about a script runner. Those are synchronous from main event loop point of view, and there can be lots of them.

Comment 4

[Florian Quèze [:florian]]

(In reply to Julien Wajsberg [:julienw] from comment #2)

Should we check at [1] if profiler_thread_is_being_profiled_for_markers() before adding this RAII?

That would work, yes. You can do it with mozilla::Maybe and .emplace (example here: https://searchfox.org/mozilla-central/rev/7a9e3bbab8f81c2cbc72a394047f948da9cfef9a/tools/profiler/public/ProfilerLabels.h#111,114)

My understanding is that most Runnable are quick so it would be better to check it here rather than in the constructor and destructor. Thoughts?

My thought when reading the question was that it would be nice to see markers for long runnables if the profiler was started during the runnable... but that case already doesn't work, so doing what you suggested wouldn't be a regression.

Comment 5

[Julien Wajsberg [:julienw]]

BTW this is happening in nightly but not release or beta:
https://searchfox.org/mozilla-central/rev/8e9b4484408154b80d7ede9e1b035819fda48fd2/xpcom/threads/nsThreadUtils.h#369-371

So I'm not sure this should block speedometer 3.

I'm still working on this though.

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: Bug 1813310 - Do not instanciate the Profiler Runnable RAII if the profiler isn't running r=florian

In Bug 1688300, we implemented that we emit markers when a Runnable
runs. But this has some performance consequences because we instanciate
the RAII even when the profiler doesn't run, even though we don't do
anything in it. Especially we're running TimeStamp::Now() which can be
slow on some platforms.

This patch avoids this by instanciating the RAII only when the profiler
runs.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1ddb6a2c5e1
Do not instanciate the Profiler Runnable RAII if the profiler isn't running r=florian

Comment 8

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/e1ddb6a2c5e1