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

Use ropes for all backends #672

Merged
merged 2 commits into from
Jun 20, 2022
Merged

Use ropes for all backends #672

merged 2 commits into from
Jun 20, 2022

Conversation

masterleinad
Copy link
Collaborator

I see up to 10% faster results for our performance benchmark and around 17% for DBScan.

@masterleinad
Copy link
Collaborator Author

@masterleinad
Copy link
Collaborator Author

Looks like Serial (and OpenMP if it can be trusted) are 10% faster on the performance regression tester for radius_[callback_]search.

@aprokop aprokop added this to the Tentative 1.3 Release milestone May 3, 2022
@aprokop aprokop added the performance Something is slower than it should be label May 9, 2022
@aprokop
Copy link
Contributor

aprokop commented Jun 15, 2022

Results from Crusher and Summit. Other than a single OpenMP case on Crusher (BM_radius_search<ArborX::BVH<OpenMP>>/10000/10000/10/0/0/1/3), which is 40% slower, everything else seems to not make much of a difference.

sycl_ropes.zip

I think it should be fine to remove the non-rope code.

@masterleinad
Copy link
Collaborator Author

I think it should be fine to remove the non-rope code.

So we can merge this?

@aprokop
Copy link
Contributor

aprokop commented Jun 17, 2022

So we can merge this?

If we do remove the non-ropes, I think it should be done fully. That means removing the corresponding node type, and the corresponding traversal. Would also simplify certain places in the construction and traversal where we do SFINAE depending on the node type.

I'm not sure if everyone agrees on that, and whether there is a sentiment to keep a node with two children just in case we need it in the future.

@masterleinad
Copy link
Collaborator Author

I thought we were saying to do this first and do the other clean-up later if using ropes is not slower.
I think it's sensible to keep the Node infrastructure we have in case we want to play with something else at the very least.
I don't think that it's necessary to remove NodeWithTwoChildren completely but I would also be fine with that.

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 fine with the change proposed. I can do the code cleanup next week.

PS make sure you update the PR title line

@masterleinad masterleinad changed the title Also use ropes for SYCL Use ropes for all backends Jun 17, 2022
@masterleinad
Copy link
Collaborator Author

I am fine with the change proposed. I can do the code cleanup next week.

I really don't care much either way if you do it or I but I guess you have specific plans in mind.

I am fine with the change proposed. I can do the code cleanup next week.

Done.

@masterleinad
Copy link
Collaborator Author

Retest this please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Something is slower than it should be
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants