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

Limit recursion-depth in dataflow to a configurable constant #1611

Merged
merged 8 commits into from Jun 24, 2015

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 15, 2015

No description provided.

@hkaiser hkaiser force-pushed the limit_dataflow_recursion branch 3 times, most recently from 55b70f3 to 00cdb1a Compare June 16, 2015 22:30
- Applying general fix to limit recursion depth while executing continuations
- Adding test triggering stack overflows (without this patch)
- Fly-by changes in future_data avoiding unneeded refcnt incref/decref
- This also applies similar changes to all related spots, needs a thorough code review.
  Essentially, after calling execute_deferred() the future may have become ready, thus
  it needs not to be handled by facilities like dataflow(), when_all(), etc.
@hkaiser
Copy link
Member Author

hkaiser commented Jun 22, 2015

This is open for a week now. Please comment asap.

}
#endif
#endif
}}

///////////////////////////////////////////////////////////////////////////////
Copy link
Member

Choose a reason for hiding this comment

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

The changes here look unrelated to the recursion depth. Can we have them in a different changeset?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are unrelated indeed, sorry for those slipping in. They are the result of the necessity to resolve some merge conflicts, things wouldn't compile otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

These changes have been applied to master as well, please disregard them here.

@sithhell
Copy link
Member

The changes to when_XXX and wait_XXX look unrelated to the issue that this should fix as well.

@hkaiser
Copy link
Member Author

hkaiser commented Jun 22, 2015

The changes to when_XXX and wait_XXX look unrelated to the issue that this should fix as well.

They are not unrelated. If the futures are deferred, then these changes avoid unlimited recursion inside the when_XXX and wait_XXX functions.

@sithhell
Copy link
Member

Looks good to me as it seems to fix the issues we currently have. However, those issues might reappear if the limit isn't small enough, for example when a large amount of stack space was previously allocated. I think this needs further thinking instead of trusting on some "magic" number, but the changes are good to be merged

@hkaiser
Copy link
Member Author

hkaiser commented Jun 22, 2015

Looks good to me as it seems to fix the issues we currently have. However, those issues might reappear if the limit isn't small enough, for example when a large amount of stack space was previously allocated. I think this needs further thinking instead of trusting on some "magic" number, but the changes are good to be merged

I agree, but don't have a better solution at this point.

hkaiser added a commit that referenced this pull request Jun 24, 2015
Limit recursion-depth in dataflow to a configurable constant
@hkaiser hkaiser merged commit 9fa57f8 into master Jun 24, 2015
@hkaiser hkaiser deleted the limit_dataflow_recursion branch June 24, 2015 13:35
@sithhell
Copy link
Member

Reverting this merge as it makes tests failed which are directly related to the changes proposed in this PR. Please fix them before having it on master. Here are the failing tests:
http://hermione.cct.lsu.edu/builders/hpx_appleclang_6_0_boost_1_56_osx_x86_64_debug/builds/236/steps/run_regression_tests/logs/tests.regressions.lcos.dataflow_recursion_1613%20%2810.76%20sec%29
http://hermione.cct.lsu.edu/builders/hpx_appleclang_6_0_boost_1_56_osx_x86_64_release/builds/238/steps/run_regression_tests/logs/tests.regressions.lcos.dataflow_recursion_1613%20%280.75%20sec%29
http://hermione.cct.lsu.edu/builders/hpx_clang_3_4_boost_1_54_debian_x86_64_debug/builds/525/steps/run_regression_tests/logs/tests.regressions.lcos.dataflow_recursion_1613%20%280.28%20sec%29
http://hermione.cct.lsu.edu/builders/hpx_clang_3_4_boost_1_54_debian_x86_64_release/builds/524/steps/run_unit_tests/logs/tests.unit.parallel.service_executors%20%280.36%20sec%29
http://hermione.cct.lsu.edu/builders/hpx_clang_3_4_boost_1_55_debian_x86_64_debug/builds/521/steps/run_regression_tests/logs/tests.regressions.lcos.dataflow_recursion_1613%20%281.54%20sec%29
http://hermione.cct.lsu.edu/builders/hpx_clang_3_4_boost_1_55_debian_x86_64_release/builds/496/steps/run_unit_tests/logs/tests.unit.parallel.service_executors%20%280.42%20sec%29
http://hermione.cct.lsu.edu/builders/hpx_clang_3_4_boost_1_56_debian_x86_64_debug/builds/449/steps/run_regression_tests/logs/tests.regressions.lcos.dataflow_launch_775%20%28Timeout%29
http://hermione.cct.lsu.edu/builders/hpx_clang_3_4_boost_1_56_debian_x86_64_debug/builds/449/steps/run_regression_tests/logs/tests.regressions.lcos.dataflow_recursion_1613%20%280.31%20sec%29

and some more.

@hkaiser hkaiser restored the limit_dataflow_recursion branch June 28, 2015 12:33
@hkaiser hkaiser deleted the limit_dataflow_recursion branch June 29, 2015 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants