mozilla::PerformanceCounter in TimeStamp_windows.cpp locking showing up in Speedometer profiles
Categories
(Core :: mozglue, enhancement)
Tracking
()
People
(Reporter: dthayer, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [sp3])
I'm seeing a decent amount of Rtl(Enter|Exit)CriticalSection
in profiling speedometer when getting the current TimeStamp
. This corresponds to this bit of TimeStamp_windows.cpp. It looks like we could just make that static variable thread_local
and do away with the locking entirely?
Comment 1•2 years ago
|
||
Doug, can you post the profile that you saw this in?
Comment 2•2 years ago
|
||
Using a thread_local
will make it so that timestamps across threads can still run into non-monotonic behaviour. In bug 1487778 comment 29 Ehsan suggests that we just limit the locking to CPUs where we see the non-monotonic behaviour. He filed bug 1515155 about doing that.
Reporter | ||
Comment 3•2 years ago
|
||
WFM - I'd be happy to just dupe this to that then so that it's directly under the speedometer3 meta.
Updated•2 years ago
|
Updated•2 years ago
|
Comment 4•1 year ago
|
||
As specified in https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables
Variables declared at block scope with the specifier static or thread_local (since C++11) have static or thread (since C++11) storage duration but are initialized the first time control passes through their declaration (unless their initialization is zero- or constant-initialization, which can be performed before the block is first entered).
In that case, we have a default initialized scalar variable (looks like decltype(LARGE_INTEGER::QuadPart)
is a scalar, right?), which means no lock associated to that static initialization.
Maybe the lock could be replaced by an atomic operation?
Comment 5•1 year ago
|
||
Again, fixing bug 1515155 is going to be a better solution than having an atomic for the vast majority of users
Comment 6•1 year ago
|
||
Using atomic operations instead of a lock seems like a low-hanging-fruit, and I don't think that it's mutually exclusive with bug 1515155 either, right?
For fun I'm going to run a quick test where I just remove the locking altogether (and don't use atomic operations at all) and see what kind of improvement I can measure to get an idea of what might be possible.
Comment 7•1 year ago
|
||
My experiment seems to imply that the noise is bigger than any impact that is coming from the critical section.
Comment 8•1 year ago
|
||
I should probably revise the conclusion that I stated above - the experiment indicates that there likely is a small win to be had here but that that win is probably small and similar in magnitude to the amount of noise that we have right now.
Description
•