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

Fence ArborX calls in the benchmark and (optionally) DBSCAN #450

Merged
merged 4 commits into from
Jan 8, 2021

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Dec 31, 2020

It may happen that the last kernel in ArborX is a Kokkos::parallel_for.
As ArborX does have a guaranteed fence() at the end, what could happen
is that that kernel is launched, and ArborX returns. Thus, when the
ending timer is called, the kernel has not been completed yet. Thus, the
measured time would be wrong in this case.

It may happen that the last kernel in ArborX is a Kokkos::parallel_for.
As ArborX does have a guaranteed fence() at the end, what could happen
is that that kernel is launched, and ArborX returns. Thus, when the
ending timer is called, the kernel has not been completed yet. Thus, the
measured time would be wrong in this case.
@aprokop aprokop added the testing Anything to do with tests and CI label Dec 31, 2020
@aprokop
Copy link
Contributor Author

aprokop commented Dec 31, 2020

The reason why I also added fences prior to ArborX calls in the loop is to make sure that if now or in the future we add some setup inside the loop, it does not creep in the timers.

@aprokop
Copy link
Contributor Author

aprokop commented Dec 31, 2020

This PR also fixes #446, as a side effect. It is still not clear what caused that hanging, though. A guess could be that in this mode we issued too many kernel launches (essentially, we continuously issued launches in the loop), and something happened when we issued too many of them.

@aprokop
Copy link
Contributor Author

aprokop commented Dec 31, 2020

So far, I have not observed any significant changes to timings on my workstation, but I've launched some jobs on Summit for observation.

@aprokop
Copy link
Contributor Author

aprokop commented Dec 31, 2020

There's no difference on Summit, either 🤷
Still, I think it's the correct way to test, and should be merged.

@aprokop aprokop requested a review from dalg24 December 31, 2020 00:41
@aprokop aprokop changed the title Fence ArborX calls in the benchmark Fence ArborX calls in the benchmark and (optionally) DBSCAN Dec 31, 2020
Copy link
Collaborator

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Otherwise, this looks reasonable.

examples/dbscan/ArborX_DBSCAN.hpp Outdated Show resolved Hide resolved
examples/dbscan/ArborX_DBSCAN.hpp Outdated Show resolved Hide resolved
examples/dbscan/ArborX_DBSCAN.hpp Outdated Show resolved Hide resolved
examples/dbscan/ArborX_DBSCAN.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I am not a big fan of the timing lambdas you introduced

benchmarks/bvh_driver/bvh_driver.cpp Show resolved Hide resolved
examples/dbscan/ArborX_DBSCAN.hpp Show resolved Hide resolved
examples/dbscan/ArborX_DBSCAN.hpp Outdated Show resolved Hide resolved
examples/dbscan/ArborX_DBSCAN.hpp Outdated Show resolved Hide resolved
@aprokop
Copy link
Contributor Author

aprokop commented Jan 8, 2021

Anything else you guys want to be addressed here?

Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I still don't like your lambda but the rest is fine. Make sure it does not get copy/pasted elsewhere in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Anything to do with tests and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants