Moved SafeWaitHandle and cancellationToken to shared by Anipik · Pull Request #18662 · dotnet/coreclr

Anipik · 2018-06-26T23:01:51Z

stephentoub · 2018-06-27T00:20:55Z

src/System.Private.CoreLib/shared/Microsoft/Win32/SafeHandles/SafeWaitHandle.cs

-            return Win32Native.CloseHandle(handle);
+#if CORECLR && PLATFORM_UNIX
+            WaitSubsystem.DeleteHandle(handle);
+		    return true


Nit: indenting is off

stephentoub · 2018-06-27T00:21:50Z

src/System.Private.CoreLib/shared/Microsoft/Win32/SafeHandles/SafeWaitHandle.cs

+        protected override bool ReleaseHandle()
        {
-            return Win32Native.CloseHandle(handle);
+#if CORECLR && PLATFORM_UNIX


Can we please avoid adding PLATFORM_UNIX and use !PLATFORM_WINDOWS instead? I'd like to keep things consistent across the codebase, and to me it's confusing when multiple constants are used for the same purpose.

stephentoub · 2018-06-27T00:23:26Z

src/System.Private.CoreLib/shared/Microsoft/Win32/SafeHandles/SafeWaitHandle.cs

        {
-            return Win32Native.CloseHandle(handle);
+#if CORECLR && PLATFORM_UNIX
+            WaitSubsystem.DeleteHandle(handle);


Why is coreclr being changed to use WaitSubsystem.DeleteHandle here?

stephentoub · 2018-06-27T00:24:35Z

src/System.Private.CoreLib/shared/System/Threading/CancellationToken.cs

+        /// <exception cref="T:System.ObjectDisposedException">The associated <see
+        /// cref="T:System.Threading.CancellationTokenSource">CancellationTokenSource</see> has been disposed.</exception>
+#if CORECLR
+        public


Why is this being made public on coreclr?

stephentoub · 2018-06-27T00:26:01Z

src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs

+
+        private bool _disposed;
+
+        // we track the running callback to assist ctr.Dispose() to wait for the target callback to complete.


CancellationTokenSource in coreclr is newer than the one in corert, and these fields were all reordered in coreclr... I'd prefer if we use the coreclr ordering / commenting / etc.

stephentoub · 2018-06-27T00:26:30Z

src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs


                // Lazily-initialize the handle.
-                var mre = new ManualResetEvent(false);
+                ManualResetEvent mre = new ManualResetEvent(false);


Nit: var was fine

stephentoub · 2018-06-27T00:26:57Z

src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs

        /// The <see cref="ExecutionContext"/> that was captured when each callback was registered
        /// will be reestablished when the callback is invoked.
-        /// </para>
+		/// </para>


The indenting here was better before.

stephentoub · 2018-06-27T00:29:13Z

src/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs

        {
            Debug.Assert(IsCancellationRequested, "ExecuteCallbackHandlers should only be called after setting IsCancellationRequested->true");
-
+            Debug.Assert(ThreadIDExecutingCallbacks != -1, "ThreadIDExecutingCallbacks should have been set.");


Are you sure this assert is valid? It looks wrong to me.

stephentoub · 2018-06-27T00:30:49Z

src/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs

                Debug.Assert(setSuccessfully, "Should have been able to complete task");
            }
-
-            void IThreadPoolWorkItem.MarkAborted(ThreadAbortException tae) { /* nop */ }


This is still part of the interface and can't be removed. Does this compile?

Yes it compiles perfectly fine. should I add it again ?

should I add it again ?

Yes, it should be added back. I expect it's picking up the base type's implementation of the interface, and that's why it's still compiling successfully without this, but in doing so removing this line is potentially changing the behavior here (even though thread aborts are deprecated and in practice we shouldn't be hitting them outside of a debugging environment).

jkotas · 2018-06-27T01:58:31Z

src/System.Private.CoreLib/shared/Microsoft/Win32/SafeHandles/SafeWaitHandle.cs

        }

-        override protected bool ReleaseHandle()
+        protected override bool ReleaseHandle()


This would be nice to split into .Windows.cs and .Unix.cs files.

jkotas · 2018-06-27T02:03:40Z

src/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs

        public Task WaitAsync()
        {
-            return WaitAsync(Timeout.Infinite, default);
+            return WaitAsync(Timeout.Infinite, default(CancellationToken));


This was removed in #17451 . Why add it back?

jkotas · 2018-06-27T02:45:21Z

src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs

        // threading and decrements it back after that. It is used as flag for the release call to know if there are
        // waiting threads in the monitor or not.
-        private int m_waitCount;
+        private volatile int m_waitCount;


Why does this need to be volatile?

It will be accessed by the multiple threads as it keeps the record of the waiting threads present in the monitor

It is always accessed under a lock as far as I can tell. Did you happen to trace down when was this volatile added or removed?

yeah you are right. I missed the lock part. It was removed here #13766 due to the reason you mentioned

jkotas · 2018-06-27T02:47:13Z

src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs

+                // spin, contention, etc. The usual number of spin iterations that would otherwise be used here is increased to
+                // lessen that extra expense of doing a proper wait.
+                var spinner = new SpinWait();
+#if CORECLR


Is this ifdef really required? Looks like a performance fix that did not get ported to CoreRT.

yeah you are right. 0799da2

is checking the m_currentCount == 0 in if condition rather than in while condition has some effect on performance ?

m_currentCount == 0 in if condition rather than in while condition has some effect on performance

That alone - no. However, there are other differences.

Just keep the CoreCLR impl here. It is the right one.

Anipik · 2018-06-27T05:46:21Z

@jkotas is it ready to merge ?

jkotas · 2018-06-27T05:55:51Z

It looks fine to me, but it would be nice to wait till morning in case Stephen has more comments.

jkotas · 2018-06-27T05:58:07Z

src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs

@@ -578,7 +565,7 @@ public Task<bool> WaitAsync(int millisecondsTimeout)
        /// </exception>
        /// <exception cref="T:System.ArgumentOutOfRangeException">
        /// <paramref name="timeout"/> is a negative number other than -1 milliseconds, which represents 
-        /// an infinite time-out -or- timeout is greater than <see cref="System.int.MaxValue"/>.


Nit: The original version of this comment looked right. The new version does not make sense to me.

Anipik · 2018-06-27T18:13:05Z

@stephentoub can I go ahead and merge it ?

…18662) * Moving SafeWaitHandle to shared * Moved CancellationToken to shared * _ remove from variable names * Default change to Default(Token) and minor changes * Fixing coreclr unix build * moving semaphoreSlim to shared * splitting safeWaitHandle to .windows and .unix Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

stephentoub · 2018-06-28T02:33:26Z

Sorry, just saw the question to me... LGTM.

Anipik · 2018-06-28T02:35:49Z

np :)

…18662) * Moving SafeWaitHandle to shared * Moved CancellationToken to shared * _ remove from variable names * Default change to Default(Token) and minor changes * Fixing coreclr unix build * moving semaphoreSlim to shared * splitting safeWaitHandle to .windows and .unix Commit migrated from dotnet/coreclr@11c9d85

Anipik mentioned this pull request Jun 26, 2018

[JustForView]Moved SafeWaitHandle and cancellationToken to shared dotnet/corert#6020

Closed

Anipik requested review from danmoseley, jkotas and maryamariyan June 26, 2018 23:04

stephentoub reviewed Jun 27, 2018

View reviewed changes

Anipik added 5 commits June 26, 2018 18:07

Moving SafeWaitHandle to shared

970ecb4

Moved CancellationToken to shared

c317c4a

_ remoev from variable names

b7712ea

Default change to Default(Token) and minor changes

b4cbe54

Fixing coreclr unix build

a0f7446

jkotas reviewed Jun 27, 2018

View reviewed changes

Anipik added 2 commits June 26, 2018 19:43

moving semaphoreSlim to shared

6573189

splitting safeWaitHandle to .windows and .unix

f7e626d

jkotas reviewed Jun 27, 2018

View reviewed changes

Change reverted

4def886

jkotas approved these changes Jun 27, 2018

View reviewed changes

jkotas reviewed Jun 27, 2018

View reviewed changes

Comment corrected

29da569

jkotas merged commit 11c9d85 into dotnet:master Jun 27, 2018

lewurm mentioned this pull request Feb 1, 2019

[2018-08] Bump corert mono/mono#12721

Merged

Anipik deleted the safeWaitHandle branch April 8, 2019 18:49

sdmaclea mentioned this pull request Feb 4, 2020

Managed Renames for consistency dotnet/runtime#28819

Merged


		private bool _disposed;

		// we track the running callback to assist ctr.Dispose() to wait for the target callback to complete.

Conversation

Anipik commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jun 27, 2018

Uh oh!

jkotas commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jun 27, 2018

Uh oh!

stephentoub commented Jun 28, 2018

Uh oh!

Anipik commented Jun 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Anipik commented Jun 26, 2018 •

edited

Loading

Anipik Jun 27, 2018 •

edited

Loading