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

Small fixes to COPY_DATA #22

Merged
merged 4 commits into from
Feb 5, 2024
Merged

Small fixes to COPY_DATA #22

merged 4 commits into from
Feb 5, 2024

Conversation

awnawab
Copy link
Collaborator

@awnawab awnawab commented Jan 24, 2024

This PR applies two small fixes to COPY_DATA:

  • The use of the OpenACC runtime function ACC_DEVICEPTR in COPY_DIM_CONTIGUOUS is replaced with acc host_data to align it with COPY_2D
  • The case of the fully contiguous field is promoted to the top of the nested SELECT CASE. This is the best possible scenario for data transfers and we should check this first rather than at the end.

The last point resulted in a more than 10% gain in data transfer speeds for CLOUDSC

@FussyDuck
Copy link

FussyDuck commented Jan 24, 2024

CLA assistant check
All committers have signed the CLA.

@pmarguinaud
Copy link
Collaborator

pmarguinaud commented Jan 24, 2024

So what you mean is that we are spending a lot of time in the nested SELECT CASE constructs ? If so, we should probably take some more radical action, like storing a pointer to the copy function in the object itself.

See for instance : https://github.com/pmarguinaud/field_api/pull/new/naan-copy-data-fix

@awnawab
Copy link
Collaborator Author

awnawab commented Jan 24, 2024

Yes I think there was definitely an overhead for the nested SELECT CASE constructs. I really like your solution with determining the copy method at field construction time and storing a pointer to it, thanks so much for suggesting it 😄 Could I please merge that commit to this PR and test it?

@awnawab
Copy link
Collaborator Author

awnawab commented Jan 25, 2024

Hi @pmarguinaud,

I tested your COPY_FUNC fix and it indeed restores all the lost performance, thanks again for that! Could I please merge your naan-copy-data-fix branch to this PR? I need another small fix on top of that for transferring very large arrays. After that we can close this PR.

@awnawab
Copy link
Collaborator Author

awnawab commented Feb 2, 2024

Hi @pmarguinaud. Thanks again for contributing the fix. Could I please merge your contribution to this PR (and add another small commit on top) so we can close this soon?

@pmarguinaud
Copy link
Collaborator

Hello Ahmad,

Yes please merge my branch in your PR.

Regards

@awnawab awnawab force-pushed the naan-copy-data-fix branch from 08eaf2e to 14eb55b Compare February 2, 2024 11:34
Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks both for fixing this!

@awnawab awnawab merged commit 5be4181 into main Feb 5, 2024
7 checks passed
@awnawab awnawab deleted the naan-copy-data-fix branch February 5, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants