Reapply revert of https://github.com/dotnet/runtime/pull/97227, fix Lock's waiter duration computation

[kouvel]

PR #97227 introduced a tick count masking issue where the stored waiter start time excludes the upper bit from the ushort tick count, but comparisons with it were not doing the appropriate masking. This was leading to a lock convoy on some heavily contended locks once in a while due to waiters incorrectly appearing to have waited for a long time.

• The first commit reapplies Add an internal mode to Lock to have it use non-alertable waits #97227 after it was reverted by Revert "Add an internal mode to Lock to have it use non-alertable w… #98867. Fixes Update some of NativeAOT's uses of Lock to use non-alertable waits #97105.
• The second commit fixes 20%+ throughput regression in goldilocks scenarios #98021 after that change

[ghost]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

PR #97227 introduced a tick count masking issue where the stored waiter start time excludes the upper bit from the ushort tick count, but comparisons with it were not doing the appropriate masking. This was leading to a lock convoy on some heavily contended locks once in a while due to waiters incorrectly appearing to have waited for a long time.

• The first commit reapplies Add an internal mode to Lock to have it use non-alertable waits #97227 after it was reverted by Revert "Add an internal mode to Lock to have it use non-alertable w… #98867. Fixes Update some of NativeAOT's uses of Lock to use non-alertable waits #97105.
• The second commit fixes 20%+ throughput regression in goldilocks scenarios #98021 after that change

Author:	kouvel
Assignees:	kouvel
Labels:	area-System.Threading
Milestone:	9.0.0

[jkotas]

Suggested change

	(Environment.TickCount & 0x7fff) - waiterStartTimeMs >= MaxDurationMsForPreemptingWaiters;
	(ushort)(Environment.TickCount - waiterStartTimeMs) >= MaxDurationMsForPreemptingWaiters;

to deal better with wrap around?

The way you have it written, we can get a false positive every 32s since (Environment.TickCount & 0x7fff) can be more than MaxDurationMsForPreemptingWaiters, waiterStartTimeMs can be near short.MaxValue, and (Environment.TickCount & 0x7fff) - waiterStartTimeMs will be large negative value that will make the condition false even through the elapsed time is more than MaxDurationMsForPreemptingWaiters.

[kouvel]

Thanks, fixed. I think the & 0x7fff is needed on the computed duration, since the recorded start time excludes the upper bit of a ushort. For instance, if the actual start time was 0xffff, 0x7fff would be recorded, and if the current time is 0x10000, the diff would be 0x8001 ms instead of 1 ms.

[jkotas]

Suggested change

	(ushort)((Environment.TickCount - waiterStartTimeMs) & 0x7fff) >= MaxDurationMsForPreemptingWaiters;
	((Environment.TickCount - waiterStartTimeMs) & 0x7fff) >= MaxDurationMsForPreemptingWaiters;

The cast to ushort is unnecessary.

I think that the ushort WaiterStartTimeMs property is misleading given that only 15 bits are actually valid. The type of the property can be int with a comment that only lower 15 bits are valid. Nearly all callers need to be aware of it.

There is another subtle bug a few lines above:

            if (currentTimeMs == 0)
            {
                // Don't record zero, that value is reserved for indicating that a time is not recorded
                currentTimeMs--;
            }

This won't work correctly when currentTimeMs is 0x8000. It is going to be recorded as 0 that is not correct.

[kouvel]

Thanks again, fixed

[VSadov]

I wonder if using the lowermost bit as the flag (as in #97227 (comment)) would make this simpler?

At least ushort WaiterStartTimeMs would not be misleading as the whole ushort range would be in use.
Also wrap around would happen in 60 sec. not in 30.

[VSadov]

        private ushort WaiterStartTimeMs
        {
            get => (ushort)(_waiterStartTimeMsAndFlags &  ~1);
            set => _waiterStartTimeMsAndFlags = (ushort)(value | (_waiterStartTimeMsAndFlags & 1));
        }

Then the same comparison as before might work:

(ushort)Environment.TickCount - WaiterStartTimeMs >= MaxDurationMsForPreemptingWaiters;

[kouvel]

That would work, but would need to make sure that 0 is not recorded when the actual time is 1, so there's still a little bit of complication at the caller side. I was thinking I could just use a bit in the _state field to simplify. The waiter count has plenty of bits and one could be used for this instead. The waiter start time is stored and used in only a couple of places so it's probably ok as is, but I'll go ahead and make that change to use a bit in the _state field, it would be a bit cleaner.

[kouvel]

Also the previous code when the ushort field was being used directly:

(ushort)Environment.TickCount - waiterStartTimeMs >= MaxDurationMsForPreemptingWaiters

Doesn't work because of the implicit int promotion in the subtract. Eg. if the recorded start time was (ushort)-100 and the current time as a ushort is 0, the result of the subtract would be a large negative int value instead of 100, and would not trigger the heuristic. Need additional casting here to force ushort math.

[kouvel]

Maybe using short instead would simplify, as the int promotion would sign-extend instead, will consider

[kouvel]

That wouldn't work either, this expression would need extra casting it seems

[VSadov]

Yes, it would need to be 16bit math.

[jkotas]

LGTM. Thanks!

[kouvel]

Updated to use a bit in the _state field instead (see #98876 (comment)), please take a look

[VSadov]

Using a bit from the _state seems cleaner.
The remaining bits are enough to track tens millions of waiters/threads, so that should be enough for a while.

[VSadov]

LGTM!

[VSadov]

If the max number of possible waiters becomes uncomfortable - i.e. if we think millions of threads will be reachable in some extreme cases, I think more bits could be taken from the spinner count.
(Actually, I think we would be ok if we were not tracking the number of spinners at all - that is 7 more free bits, if needed)

[kouvel]

Rebased