{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/137","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/137/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/137/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/137/events","html_url":"https://github.com/dotnet/coreclr/pull/137","id":56921086,"number":137,"title":"SemaphoreSlim performance improvement","user":{"login":"ikopylov","id":10262823,"avatar_url":"https://avatars.githubusercontent.com/u/10262823?v=3","gravatar_id":"","url":"https://api.github.com/users/ikopylov","html_url":"https://github.com/ikopylov","followers_url":"https://api.github.com/users/ikopylov/followers","following_url":"https://api.github.com/users/ikopylov/following{/other_user}","gists_url":"https://api.github.com/users/ikopylov/gists{/gist_id}","starred_url":"https://api.github.com/users/ikopylov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ikopylov/subscriptions","organizations_url":"https://api.github.com/users/ikopylov/orgs","repos_url":"https://api.github.com/users/ikopylov/repos","events_url":"https://api.github.com/users/ikopylov/events{/privacy}","received_events_url":"https://api.github.com/users/ikopylov/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dotnet/coreclr/labels/cla-signed","name":"cla-signed","color":"207de5"},{"url":"https://api.github.com/repos/dotnet/coreclr/labels/performance","name":"performance","color":"5319e7"},{"url":"https://api.github.com/repos/dotnet/coreclr/labels/Threading","name":"Threading","color":"bfe5bf"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/dotnet/coreclr/milestones/5","html_url":"https://github.com/dotnet/coreclr/milestones/Future","labels_url":"https://api.github.com/repos/dotnet/coreclr/milestones/5/labels","id":1328081,"number":5,"title":"Future","description":"Milestone tracking the issues that have been triaged for after 1.0.0-rtm","creator":{"login":"joshfree","id":1302850,"avatar_url":"https://avatars.githubusercontent.com/u/1302850?v=3","gravatar_id":"","url":"https://api.github.com/users/joshfree","html_url":"https://github.com/joshfree","followers_url":"https://api.github.com/users/joshfree/followers","following_url":"https://api.github.com/users/joshfree/following{/other_user}","gists_url":"https://api.github.com/users/joshfree/gists{/gist_id}","starred_url":"https://api.github.com/users/joshfree/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/joshfree/subscriptions","organizations_url":"https://api.github.com/users/joshfree/orgs","repos_url":"https://api.github.com/users/joshfree/repos","events_url":"https://api.github.com/users/joshfree/events{/privacy}","received_events_url":"https://api.github.com/users/joshfree/received_events","type":"User","site_admin":false},"open_issues":162,"closed_issues":3,"state":"open","created_at":"2015-09-29T00:26:07Z","updated_at":"2015-11-17T15:35:54Z","due_on":null,"closed_at":null},"comments":22,"created_at":"2015-02-07T18:44:01Z","updated_at":"2015-11-18T16:29:53Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/137","html_url":"https://github.com/dotnet/coreclr/pull/137","diff_url":"https://github.com/dotnet/coreclr/pull/137.diff","patch_url":"https://github.com/dotnet/coreclr/pull/137.patch"},"body":"This tweak improves performance for the case when the number of working threads more than the number of CPU cores.\r\n\r\nThe original code uses SpinWait before calling Monitor.Enter(), but the spinning condition is not perfect. To reduce the number of unnecessary waiting on the Monitor it would be good to wait while other threads finished their work inside critical section. The idea of this tweak is to arrange threads on SpinWait so that they do not enter critical section concurrently.\r\n\r\n\r\nMeasurement taken on i7-4770k (8 cores: 4 physical + 4HT).\r\n\r\nSemaphoreSlim tests (source code of the test can be found [here](https://gist.github.com/ikopylov/3ba679adaaeb416d19a4#file-semaphoreslimperftest-cs)).\r\nOld:\r\n```\r\n<# of threads calling Release>, <# of threads calling Wait>: <Average execution time>\r\n1, 1:   1585ms\r\n8, 8:   2367ms\r\n1, 8:   3727ms\r\n1, 16: 97430ms\r\n8, 1:   1522ms\r\n16, 1:  1515ms\r\n```\r\n\r\nNew:\r\n```\r\n1, 1:   1488ms\r\n8, 8:   2402ms\r\n1, 8:   3556ms\r\n1, 16:  6649ms\r\n8, 1:   1546ms\r\n16, 1:  1489ms\r\n```\r\n\r\nFor the '**1, 16**' case the boost is obvious. For other cases the difference is comparable to the measurement error.\r\n\r\n\r\n\r\nBlockingCollection uses SemaphoreSlim under cover so I decided to test it too (source code of the test can be found [here](https://gist.github.com/ikopylov/3ba679adaaeb416d19a4#file-blockingcollectionperftest-cs)).\r\nOld:\r\n```\r\n<# of threads adding elements>, <# of threads taking elements>: <Average execution time>\r\n1, 1:    2106ms\r\n4, 4:    2111ms\r\n8, 8:    2683ms\r\n16, 1:  24670ms\r\n1, 16:  22661ms\r\n16, 16:  3147ms\r\n```\r\n\r\nNew:\r\n```\r\n1, 1:    2126ms\r\n4, 4:    1888ms\r\n8, 8:    2176ms\r\n16, 1:   2997ms\r\n1, 16:   3577ms\r\n16, 16:  2138ms\r\n```\r\n\r\nAgain you can see the measurable boost when the number of threads more than the number of cores.\r\n"},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/157768815","html_url":"https://github.com/dotnet/coreclr/pull/137#issuecomment-157768815","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/137","id":157768815,"user":{"login":"alexandrnikitin","id":1798438,"avatar_url":"https://avatars.githubusercontent.com/u/1798438?v=3","gravatar_id":"","url":"https://api.github.com/users/alexandrnikitin","html_url":"https://github.com/alexandrnikitin","followers_url":"https://api.github.com/users/alexandrnikitin/followers","following_url":"https://api.github.com/users/alexandrnikitin/following{/other_user}","gists_url":"https://api.github.com/users/alexandrnikitin/gists{/gist_id}","starred_url":"https://api.github.com/users/alexandrnikitin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alexandrnikitin/subscriptions","organizations_url":"https://api.github.com/users/alexandrnikitin/orgs","repos_url":"https://api.github.com/users/alexandrnikitin/repos","events_url":"https://api.github.com/users/alexandrnikitin/events{/privacy}","received_events_url":"https://api.github.com/users/alexandrnikitin/received_events","type":"User","site_admin":false},"created_at":"2015-11-18T16:29:53Z","updated_at":"2015-11-18T16:29:53Z","body":"Then, probably, you meant `currentCount` variable instead of `releaseCount` because as i said `releaseCount` is always 1 in your tests and you always `Pulse` once.\r\n\r\nI think the idea of `PulseAll` is to have a \"fair\" lock and provide the ability to acquire it to all threads, not only to a couple. That's why you have a lot of false wakeups.\r\nHere's an interesting piece of code in `Monitor.Enter` insides: \r\nhttps://github.com/dotnet/coreclr/blob/release/1.0.0-rc1/src/vm/syncblk.cpp#L2862\r\nWe can acquire the lock but still must wait for fairness sake. And yes, it's a waste of resources.\r\n\r\nThe idea of spins and `m_currentCount` check is to avoid \"expensive\" wait state in `Monitor.Enter()`\r\n```csharp\r\nSpinWait spin = new SpinWait();\r\nwhile (m_currentCount == 0 && !spin.NextSpinWillYield)\r\n{\r\n    spin.SpinOnce();\r\n}\r\ntry { }\r\nfinally\r\n{\r\n    Monitor.Enter(m_lockObj, ref lockTaken);\r\n    if (lockTaken)\r\n    {\r\n        m_waitCount++;\r\n    }\r\n}\r\n```\r\n\r\nIn my opinion the \"problem\" here is in contention for `m_lockObj` between releasers and waiters."}}