Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose a Lock type in preview mode #87672

Merged
merged 17 commits into from
Oct 30, 2023
Merged

Expose a Lock type in preview mode #87672

merged 17 commits into from
Oct 30, 2023

Conversation

kouvel
Copy link
Member

@kouvel kouvel commented Jun 16, 2023

  • For now, the Lock type requires preview features to be enabled
  • Ported CoreCLR's AwareLock implementation to C# with a bit of refactoring, folded in a couple of ideas from NativeAOT's previous Lock implementation, and fixed a couple of issues.
  • Added an adaptive spin strategy to reduce CPU time from spin-waiting when spin-waits are not effective
  • This implementation replaces NativeAOT's Lock implementation. The performance of acquiring a lock under contention is improved in NativeAOT.

API review: #34812
Tracking issue: #87673

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 16, 2023

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

Issue Details
  • For now, the Lock type requires preview features to be enabled
  • Ported CoreCLR's AwareLock implementation to C# with a bit of refactoring, folded in a couple of ideas from NativeAOT's previous Lock implementation, and fixed a couple of issues.
  • Added an adaptive spin strategy to reduce CPU time from spin-waiting when spin-waits are not effective
  • This implementation replaces NativeAOT's Lock implementation. The performance of acquiring a lock under contention is improved in NativeAOT.

API review: #34812

Author: kouvel
Assignees: kouvel
Labels:

area-System.Threading

Milestone: 8.0.0

@kouvel kouvel mentioned this pull request Jun 16, 2023
8 tasks
@vargaz
Copy link
Contributor

vargaz commented Jun 16, 2023

The mono changes look ok.

@kouvel
Copy link
Member Author

kouvel commented Jun 16, 2023

Here are some perf numbers on NativeAOT. I used a test that has multiple threads enter a lock, perform a short delay, exit the lock, and perform another short delay. Throughput is the number of enter+delay+exit+delay rounds performed per ms. The delay is a recursive calculation of a random Fibonacci number between 4 and 9. The numbers are from my Intel x64 6-core 12-thread machine on Windows.

Using Monitor:

Threads Throughput before CPU usage before (%) Throughput after CPU usage after (%)
1 6800 9.3 6800 9.1
2 5700 16.9 5700 16.3
3 5100 23.8 5300 23.5
6 3900 44.9 5400 27.5
12 2500 56.3 5300 24.5
24 2600 64.8 5300 24.9

Using Lock:

Threads Throughput CPU usage (%)
1 6800 8.4
2 6400 16.0
3 5800 23.9
6 5500 28.5
12 5500 26.1
24 5500 24.8

return true;
}

throw new LockRecursionException(SR.Lock_Enter_LockRecursionException);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'm following how this gets thrown. It would mean _recursionCount was -1. I don't see a test in LockTests for LockRecursionException.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh - you're using 32 bit overflow like Native AOT .. since this is a new type, how about picking something smaller that is still always going to mean a bug in the app, but small enough that it won't cause a hang in the app (and also becomes feasibly unit testable). Eg., 65K

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact that's what the comment suggests "is expected to be high enough that it would typically not be reached when the lock is used properly."

@kouvel
Copy link
Member Author

kouvel commented Jun 17, 2023

Rebased to fix conflicts

@VSadov
Copy link
Member

VSadov commented Jun 19, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kouvel
Copy link
Member Author

kouvel commented Oct 28, 2023

Trying a rebase due to some unrelated build failures.

@kouvel
Copy link
Member Author

kouvel commented Oct 30, 2023

The remaining issues in the CI appear to be unrelated.

@kouvel kouvel merged commit d58dd91 into dotnet:main Oct 30, 2023
183 of 188 checks passed
@kouvel kouvel deleted the Lock branch October 30, 2023 21:10
@radical
Copy link
Member

radical commented Oct 31, 2023

This was merged on Red and broke Wasm.Build.Tests on main. #94212

@radical
Copy link
Member

radical commented Oct 31, 2023

Also one of the failures is:

Unhandled exception. System.InvalidOperationException: There is no currently active test.
   at Xunit.Sdk.TestOutputHelper.QueueTestOutput(String output) in /_/src/xunit.execution/Sdk/Frameworks/TestOutputHelper.cs:line 60
   at System.Diagnostics.AsyncStreamReader.FlushMessageQueue(Boolean rethrowInNewThread)
--- End of stack trace from previous location ---
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
./RunTests.sh: line 105:    83 Aborted                 (core dumped) dotnet exec xunit.console.dll Wasm.Build.Tests.dll -xml $XHARNESS_OUT/testResults.xml $HELIX_XUNIT_ARGS -nocolor -verbose -notrait category=IgnoreForCI -notrait category=failing $XUnitTraitArg $RSP_FILE

Could this be related to the change here? I have never seen this happen on the tests before.

cc @lewing

@jkotas
Copy link
Member

jkotas commented Oct 31, 2023

Also introduced #94227

@kouvel
Copy link
Member Author

kouvel commented Oct 31, 2023

This was merged on Red and broke Wasm.Build.Tests on main. #94212
Could this be related to the change here? I have never seen this happen on the tests before.

I have no idea how this PR could cause those failures, apologies if there was a mishap. There are only a few lines of code in this PR that would affect wasm, do you see anything that could be relevant? I'll see if I can get a repro, but otherwise I don't see any link between the failure and this PR.

Also introduced #94227

#94241 should fix the issue.

@kouvel
Copy link
Member Author

kouvel commented Oct 31, 2023

Could this be related to the change here? I have never seen this happen on the tests before.

Only thing I can think of at the moment is that there is a pragma warning disable for using preview features when we should probably be enabling preview features for the corelib project. I don't know what kind of effect that would have, but I think that's something that was missed after local testing and unaddressed, so I'll fix that anyway.

kouvel added a commit to kouvel/runtime that referenced this pull request Nov 1, 2023
- Used the project property instead of disabling the compiler warning. Probably a bit cleaner, and would enable using Lock in other libraries to try it out.
- Undid an unnecessary change from dotnet#87672
kouvel added a commit that referenced this pull request Nov 2, 2023
* Small cleanup for Lock in Mono

- Enable preview features in shared CoreLib project instead of disabling the compiler warning. Probably a bit cleaner, and would enable using Lock in other libraries to try it out.
- Undid an unnecessary change from #87672
@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants