Open Bug 1802218 Opened 2 years ago Updated 1 year ago

mozilla::PerformanceCounter in TimeStamp_windows.cpp locking showing up in Speedometer profiles

Categories

(Core :: mozglue, enhancement)

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?

Doug, can you post the profile that you saw this in?

Flags: needinfo?(dothayer)

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.

Depends on: 1515155

WFM - I'd be happy to just dupe this to that then so that it's directly under the speedometer3 meta.

Flags: needinfo?(dothayer)
Whiteboard: [sp3]

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?

Again, fixing bug 1515155 is going to be a better solution than having an atomic for the vast majority of users

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.

My experiment seems to imply that the noise is bigger than any impact that is coming from the critical section.

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.

You need to log in before you can comment on or make changes to this bug.