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 --ponyminthreads default to 0 #3020

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

dipinhora
Copy link
Contributor

Thanks to @slfritchie's hard work in #2926 and #2561 to find and
fix the bugs in the dynamic scheduler scaling logic, scheduler
thread 0 now also suspends and resumes correctly without issues
(in my testing).

This commit changes the default for --ponyminthreads from 1 to
0. The last time @slfritchie ran into issues was in March and
left a comment with the commands he used at:
#2561 (comment)

I ran the commands from @slfritchie's comment (with minor changes
to ensure --ponyminthreads=0) using Pony 0.26.0 for a few hours
without any issues.

@dipinhora
Copy link
Contributor Author

The commands I used for testing are the following:

Start data receiver (from wallaroo) to listen on port 9999:
./utils/data_receiver/data_receiver --ponyminthreads=0 --ponythreads=1 --ponynopin --ponynoblock --no-write --framed --listen :9999

Run sender (from wallaroo) to send data every 10 ms to port 9999 for about 15 mins at a time. 10 ms is used to give scheduler thread 0 enough time to sleep between batches.
while true; do date; ./giles/sender/sender -h :9999 -m 45000000 -s 450 -i 10_000_000 -f ~/wallaroo/testing/data/market_spread/nbbo/350-symbols_nbbo-fixish.msg -r --ponyminthreads=0 --ponythreads=1 -y -g 46 --ponynopin -w --ponynoblock; done

Run the message-ubench (from ponyc examples) command that hung for @slfritchie repeatedly:
while true; do date; ./examples/message-ubench/message-ubench --initial-pings=1 --pingers=1 --ponythreads=32 --report-interval=10 --report-count=60 --ponynoblock --ponyminthreads=0 --ponynopin; done

@@ -197,8 +197,7 @@ static void usage(void)
" --ponythreads Use N scheduler threads. Defaults to the number of\n"
" cores (not hyperthreads) available.\n"
" --ponyminthreads Minimum number of active scheduler threads allowed.\n"
" Defaults to 1. Use of 0 is considered experimental\n"
" and may cause improper scheduler behavior.\n"
" Defaults to 0.\n"
Copy link
Member

Choose a reason for hiding this comment

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

Can we explain what zero means here?

It's not immediately clear to me and the same will likely be true for others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jemc The 0 means that it allows all scheduler threads to suspend resulting in a more efficient application with no busy polling at all when there is no work to be done (the ASIO thread is still awake and will wake up a scheduler thread when more work needs to be done either due to a timer, or network io, or some other similar event). --ponyminthreads controls the minimum number of scheduler threads that must be active (and busy polling for work), no matter what.

I'm not sure if you meant for me to include what 0 means in the help text. If so, I'm not sure of a succinct way to write that up.

Copy link
Member

@jemc jemc Feb 26, 2019

Choose a reason for hiding this comment

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

I suggest: Defaults to 0, meaning that all scheduler threads are allowed to be suspended when no work is available

@SeanTAllen
Copy link
Member

@dipinhora can you write up the release notes to be included with this?

@SeanTAllen SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Feb 25, 2019
@dipinhora
Copy link
Contributor Author

@SeanTAllen Release notes:

Change the --ponyminthreads default so that it allows all scheduler threads (as opposed to all but 1 previously) to suspend resulting in a more efficient application with no busy polling at all when there is no work to be done (the ASIO thread is still awake and will wake up a scheduler thread when more work needs to be done either due to a timer, or network io, or some other similar event).

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Feb 26, 2019
@SeanTAllen
Copy link
Member

This can be merged once the help output is updated.

Thanks to @slfritchie's hard work in ponylang#2926 and ponylang#2561 to find and
fix the bugs in the dynamic scheduler scaling logic, scheduler
thread 0 now also suspends and resumes correctly without issues
(in my testing).

This commit changes the default for `--ponyminthreads` from `1` to
`0`. The last time @slfritchie ran into issues was in March and
left a comment with the commands he used at:
ponylang#2561 (comment)

I ran the commands from @slfritchie's comment (with minor changes
to ensure `--ponyminthreads=0`) using Pony 0.26.0 for a few hours
without any issues.
@dipinhora dipinhora force-pushed the sched_0_suspend_enable branch from c7db962 to 882b89d Compare February 26, 2019 18:05
@dipinhora
Copy link
Contributor Author

Help output updated and PR rebased onto latest master.

@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Feb 26, 2019
@SeanTAllen SeanTAllen mentioned this pull request Feb 26, 2019
@SeanTAllen
Copy link
Member

I'll merge this once it passes CI

@SeanTAllen SeanTAllen merged commit d96fcb1 into ponylang:master Feb 27, 2019
ponylang-main added a commit that referenced this pull request Feb 27, 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.

4 participants