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

Change default to disable pinning of scheduler threads to CPU cores #3024

Merged
merged 1 commit into from
Feb 26, 2019

Conversation

dipinhora
Copy link
Contributor

Prior to this commit, if a random user happened to be running
multiple pony applications that are CPU intensive, they would run
into unnecessary CPU contention due to CPU pinning (at least on
Linux) due to the first few cores being the ones used by all the
applications even if the system may have more cores available. This
was particularly troublesome because users may not even be aware
that the application they're running is a pony application.

This commit changes the default to not pin scheduler threads to CPU
cores. It also removes the --ponynopin CLI argument and replaces
it with the --ponypin CLI argument to leave the control for
pinning as an option for advanced users without requiring all users
of pony applications to understand pinning and how to disable it.

Resolves #3023

Prior to this commit, if a random user happened to be running
multiple pony applications that are CPU intensive, they would run
into unnecessary CPU contention due to CPU pinning (at least on
Linux) due to the first few cores being the ones used by all the
applications even if the system may have more cores available. This
was particularly troublesome because users may not even be aware
that the application they're running is a pony application.

This commit changes the default to not pin scheduler threads to CPU
cores. It also removes the `--ponynopin` CLI argument and replaces
it with the `--ponypin` CLI argument to leave the control for
pinning as an option for advanced users without requiring all users
of pony applications to understand pinning and how to disable it.

Resolves ponylang#3023
@SeanTAllen
Copy link
Member

Big thumbs up on this.

@SeanTAllen
Copy link
Member

If this is merged, then I think the performance guide on the website needs to be updated.

@SeanTAllen
Copy link
Member

@dipinhora can you open a PR to adjust the performance section of the website to account for these changes and then we can merge this?

@SeanTAllen SeanTAllen added do not merge This PR should not be merged at this time changelog - changed Automatically add "Changed" CHANGELOG entry on merge and removed needs discussion during sync labels Feb 26, 2019
@dipinhora
Copy link
Contributor Author

@SeanTAllen ponylang/ponylang-website#394 created to update the performance guide for the --ponypin changes and also to change it for some recent cycle detector changes.

@SeanTAllen SeanTAllen merged commit b8e39ee into ponylang:master Feb 26, 2019
ponylang-main added a commit that referenced this pull request Feb 26, 2019
@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Feb 26, 2019
@SeanTAllen
Copy link
Member

Thanks @dipinhora. This is awesome.

@SeanTAllen SeanTAllen mentioned this pull request Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants