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

Optimize the scheduler, remove redundant semaphore and interlocked exchange. #2218

Merged
merged 1 commit into from Oct 2, 2016

Conversation

gabikliot
Copy link
Contributor

It would be interesting to know if this saves any perf.

We maintain this semaphore and counter only for the case that we inject more threads dynamically. We don't actually do that any more, for years (its a separate discussion why). Did not want to fully remove the thread injection, but sure no need to pay the semaphore and interlocked exchange price, on a very hot path.

It is one of those things that is hard to predict how perf. will be impacted. I won't be surprised if it does not help at all (semaphore was never blocking, it always had enough room), or saves a ton (since it still inc/dec the semaphore on every task)!

@dVakulen
Copy link
Contributor

dVakulen commented Sep 23, 2016

I think it should help to some extent, for example at https://cloud.githubusercontent.com/assets/5787619/17674725/6f621da0-632f-11e6-95f8-7e69d793a9c3.png (#2060) this semaphore appears pretty high in profiling results.

@sergeybykov sergeybykov added this to the 1.4.0 milestone Sep 23, 2016
@gabikliot
Copy link
Contributor Author

Anyone interested in reviewing/testing this? Maybe triage and assign reviewer?

@sergeybykov
Copy link
Contributor

We'll definitely review and test this soon, now that 1.3.0 is branched out and pretty much done.

@sergeybykov sergeybykov self-assigned this Sep 28, 2016
@ReubenBond
Copy link
Member

ReubenBond commented Sep 29, 2016

Changes LGTM. As a side-note, the distinction between InjectMoreWorkerThreads and ShouldInjectWorkerThread is not immediately clear from the names. A more obvious name for the former might be EnableWorkerThreadInjection.

@sergeybykov
Copy link
Contributor

I'm ready to merge this. But I like @ReubenBond's suggestion to rename InjectMoreWorkerThreads to EnableWorkerThreadInjection.

@gabikliot
Copy link
Contributor Author

Sure, will rename.

@sergeybykov
Copy link
Contributor

Or I can merge it, and submit the renaming as a separate change.

@sergeybykov sergeybykov merged commit 9cd7133 into dotnet:master Oct 2, 2016
@sergeybykov
Copy link
Contributor

Thanks, Gabi!

@gabikliot
Copy link
Contributor Author

No problem. Did it make any change to the numbers?

@sergeybykov
Copy link
Contributor

I didn't see any measurable gains.

@sergeybykov
Copy link
Contributor

BTW, please use the "Squash and merge" option instead of "Create a merge commit" when merging PRs that consist of a single commit or should be squashed, and "Rebase and merge" (new GitHub feature) for those PRs where we want to preserve the individual commits. These two options avoid merge commits, and keep the history less cluttered. Not a big deal, just nice features that GitHub recently added.

@gabikliot
Copy link
Contributor Author

Will do.

sergeybykov pushed a commit to sergeybykov/orleans that referenced this pull request Oct 27, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants