Change AsyncLazy from NonReentrantLock to a simple monitor enter

[CyrusNajmabadi]

Addresses expensive lock contention in AsyncLazy (a core type with a fair amount of contention). From:

to:

Note: TakeLock itself went from:

to

(effectively basically nothing).

I can only conclude our NonreentrantLock is simply terrible :)

--

Kicked off build here: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=9710024&view=results

[CyrusNajmabadi]

Probably good to get a distinct speedometer pass on this.

[jasonmalinowski]

@CyrusNajmabadi Does anything indicate that SemaphoreSlim just won't be spinning and instead is just being more async?

Your screenshot might also not be accurate there: the "before" in TakeLock is the System.__Canon one, but the last screenshot is the one specialized on an enum, which I would imagine would be cheap. I'd expect there's another frame somewhere else which is the thing really to compare with. If you can't find the frame at all then I'd imagine something is wrong with your trace as I can't imagine TakeLock would disappear entirely!

[sharwell]

Also the first pair of images shows an increase in the Inc column.

[sharwell]

My main concern here relates to the use of a static field (the documentation for the change to a static field states that contention was not measured to be a problem). However, in review I noticed that there is no use of asynchronous functionality for this lock. Therefore, I propose we update this to use Monitor's functionality and make it equivalent to the current NonReentrantLock.

[CyrusNajmabadi]

Therefore, I propose we update this to use Monitor's functionality and make it equivalent to the current NonReentrantLock.

i don't know what this means. We use the benefic of cancellable blocking wait on the lock. SemaphoreSlim works much better for this case.

If you'd like to work on an alternate here. Go for it.

[sharwell]

I have concerns about the specific supporting data provided in the initial post above. If it turns out that contention is a problem on this path despite prior documentation to the contrary that needs to be resolved, I would suggest an approach like shown in 8e56c49 instead of the one here.

[CyrusNajmabadi]

Contention can be negligible while this PR still being beneficial. For one, we're comparing the runtime's optimized semaphore impl, with our ancient, complex, sync primitive that was written for the workspace when we ran into a reentrancy problem (in the workspace) a decade ago). We used this as an acceptable type for AsyncLazy, but it was just the tool we had at the time.

[CyrusNajmabadi]

Comparing before/after:

It just seems like a NonreentrantLock is not very good. And why would it be? We didn't ever invest in making it good. But it's not 10 years ago. We can move to battle-tested, highly performance runtime primitives that supplant these cases.