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

Scalable barrier #2321

Merged
merged 9 commits into from Oct 26, 2016
Merged

Scalable barrier #2321

merged 9 commits into from Oct 26, 2016

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Sep 6, 2016

The barrier was implemented using a K-ary tree approach with the algorithm
as presented here: http://www.cs.fsu.edu/~asriniva/papers/HiPC15.pdf

The arity is configurable through hpx.lcos.collectives.arity and the cut off when
to switch to the tree based algorithm is hpx.lcos.collectives.cut_off.

# Conflicts:
#	hpx/runtime/agas/component_namespace.hpp
#	hpx/runtime/agas/primary_namespace.hpp
#	hpx/runtime/agas/symbol_namespace.hpp
#	src/runtime/agas/addressing_service.cpp
#	src/runtime/agas/detail/bootstrap_component_namespace.cpp
#	src/runtime/agas/detail/bootstrap_locality_namespace.cpp
#	src/runtime/agas/detail/hosted_component_namespace.cpp
#	src/runtime/agas/primary_namespace.cpp
#	src/runtime/agas/server/primary_namespace_server.cpp
#	src/runtime/agas/symbol_namespace.cpp
#	src/util/logging.cpp
@hkaiser
Copy link
Member Author

hkaiser commented Sep 6, 2016

The config settings need to be added to the docs, still.

Thomas Heller and others added 3 commits September 15, 2016 13:10
The barrier was implemented using a K-ary tree approach with the algorithm
as presented here:
http://www.cs.fsu.edu/~asriniva/papers/HiPC15.pdf

The arity is configurable through hpx.lcos.collectives.arity and the cut off when
to switch to the tree based algorithm is hpx.lcos.collectives.cut_off.
- flyby: adapt to new sync/async API
- making things compile for MSVC
- adding missing headers
- removed old barrier component
Those numbers have been determined experimentally on edison
@sithhell
Copy link
Member

sithhell commented Oct 5, 2016

Can anyone review this please?

@hkaiser
Copy link
Member Author

hkaiser commented Oct 5, 2016

It was not clear to me that the changed boundaries would fix the issue. I think there should be some code checking pre-conditions if the barrier breaks with boundaries set too low.

@sithhell
Copy link
Member

sithhell commented Oct 5, 2016

What do you mean? Which boundaries? The code is fine, the arity and cut off were chosen experimentally to get the best performance. Every value here should work

@hkaiser
Copy link
Member Author

hkaiser commented Oct 5, 2016

Sure. I trust the values used as a default now are fine. What I meant was that those values could be changed by the user and as we have seen, providing bad values not only could slow down the execution (I wouldn't care about this case, the user is not supposed to play with those values), but could break the barrier altogether (as the failing test has shown). So what I'd suggest is to add tests verifying that the selected values don't break the barrier functionally.

@sithhell
Copy link
Member

sithhell commented Oct 5, 2016

On Mittwoch, 5. Oktober 2016 11:25:33 CEST Hartmut Kaiser wrote:

Sure. I trust the values used as a default now are fine. What I meant was
that those values could be changed by the user and as we have seen,
providing bad values not only could slow down the execution (I wouldn't
care about this case, the user is not supposed to play with those values),
but could break the barrier altogether (as the failing test has shown). So
what I'd suggest is to add tests verifying that the selected values don't
break the barrier functionally.

The failing test was due to a bug in the algorithm which has been fixed.
The values have been determined only after the algorithm was fixed and the unit
test ran successfully. Did you encounter problems when testing this branch?

@hkaiser
Copy link
Member Author

hkaiser commented Oct 5, 2016

The failing test was due to a bug in the algorithm which has been fixed. The values have been determined only after the algorithm was fixed and the unit test ran successfully. Did you encounter problems when testing this branch?

Ok. This was a misunderstanding on my end, then.

LGTM, please go ahead and merge it (after resolving the conflicts, that is).

hkaiser and others added 4 commits October 12, 2016 09:59
Conflicts:
	hpx/lcos/server/barrier.hpp
	src/lcos/barrier.cpp
This change prevents the condition variable to wait in non HPX thread contexts.
@sithhell
Copy link
Member

The remaining problem has been fixed.

@hkaiser hkaiser merged commit dfb28b6 into master Oct 26, 2016
@hkaiser hkaiser deleted the scalable_barrier branch October 26, 2016 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants