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

Restrain progress messages #228

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
2 participants
@frankmcsherry
Copy link
Member

frankmcsherry commented Feb 11, 2019

This PR ports the logic from #205 to the new event-driven scheduler logic. Most of the logic is unchanged or simplified, due to the state maintained by the new reachability tracker.

One important change is that the default behavior is now with the progress accumulation optimization enabled, rather than disabled as it is in #205. To return to the old behavior, where progress messages are eagerly exchanged rather than exchanged only when justified, set the environment variable

DEFAULT_PROGRESS_MODE = EAGER

This should cause all progress messages to be exchanged as soon as they are available, even if they cannot obviously improve the progress of the computation.

The down side for non-eager progress message exchange is the potential increase of the critical path for latency sensitive, progress bound computations. Where one worker.step() could have circulated all outstanding progress messages, we may need multiple rounds in order to inform participants that they should now send their updates.

It remains an open question how painful this is, but the alternative (eager progress messages) has a known downside of many orders of magnitude increase in the volume of progress traffic. Ideally, some experimentation will begin to surface any problems, and we can look at ways to fix things further.

@frankmcsherry frankmcsherry referenced this pull request Feb 11, 2019

Closed

Progress hack #205

@comnik

This comment has been minimized.

Copy link
Contributor

comnik commented Feb 11, 2019

Is there a reason why this is configured via an environment variable and not as a compile-time feature?

And does either mode affect running in Configuration::Thread?

Thanks!

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Feb 11, 2019

Good questions both.

  1. The environment variable approach lets us better test with and without, to relate the performance of two configurations. The test is not expensive (the variable is read once per scope, and the boolean bound internally), and the compiled code are not much different. Perhaps it should be added to per-scope configuration parameters, but I think at least for experimentation the environment variable approach makes things a bit easier to test, diagnose, and debug.

  2. Good question. I think "mostly no". There might be some corner cases where it makes an interesting change. I think I could write such a demonstration (e.g. a scope whose inputs don't advance, but which contains a flatmap that slowly plays out its data; would accumulate updates until the scope input advances). If it does have a noticeable impact something is probably mis-understood on my part, though.

Not planning on landing this without further testing, and perhaps without swapping the defaults back to eager to avoid surprises for folks not paying attention to our public test schedule.

@comnik

This comment has been minimized.

Copy link
Contributor

comnik commented Feb 12, 2019

Ok, thanks for clearing things up :)

@frankmcsherry frankmcsherry merged commit 9f3716f into master Feb 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Feb 14, 2019

Merged with default changed back to old behavior. Switching default will be a more interesting choice that should call for a bit of experimentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment