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

Add a memory space abstraction #654

Closed
wants to merge 8 commits into from
Closed

Conversation

pratikvn
Copy link
Member

@pratikvn pratikvn commented Oct 20, 2020

This PR is a draft PR which showcases how a memory space abstraction would look like in Ginkgo. All the memory operations such as alloc, free and raw_copy_to's are move from the Executor to the MemorySpace class.

By default, Executors are created with separate new memory space objects thereby trying to maintain the same behaviour as before this PR. Executors can also be created with an associated memory space. In that case, with matching memory spaces, the data would not be copied.

This PR is experimental and is available for anyone who wants to discuss and play around with the code.

Please feel free to point out problems and issues with the current implementation and suggestions for improvements are welcome.

TODO

  • Add documentation.
  • Check whether polymorphic_object can also be completely abstracted.

Update:
Interface breaking change, See #402

@pratikvn pratikvn self-assigned this Oct 20, 2020
@pratikvn pratikvn added is:affects-performance This is related to something which affects performance. is:experimental This is an experimental feature/PR/issue/module. is:help-wanted Need ideas on how to solve this. is:interface-breaking This change may break interface compatibility. Can only be done through a major release. mod:all This touches all Ginkgo modules. 1:ST:WIP This PR is a work in progress. Not ready for review. labels Oct 20, 2020
@pratikvn pratikvn force-pushed the memory-space-abstraction branch 2 times, most recently from 3bc66c5 to b6129c9 Compare October 20, 2020 19:06
@pratikvn
Copy link
Member Author

Note: After thinking about this I decided that it does not really make sense to try to force this into a non-interface breaking addition. There are a couple of things that are problematic:

  1. The Loggers on_* memory logging methods currently take Executor * and those will need to be updated. For a non-interface breaking addition, I can only add additional methods which take MemorySpace *, which considerably bloats the code.

  2. The alloc , free and copy_from methods are in the public interface of the Executor class and hence I cannot simply move them to the MemorySpace class but will have to call the memory space methods from within the Executor methods, which kind of defeats the purpose in a certain sense.

So, I think maybe it is indeed better to wait to add this until 2.0.0. But I guess we could possibly merge this into the ginkgo-2.0.0 branch.

@sonarcloud
Copy link

sonarcloud bot commented Oct 21, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 65 Code Smells

41.2% 41.2% Coverage
2.4% 2.4% Duplication

warning The version of Java (1.8.0_121) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #654 into develop will decrease coverage by 0.29%.
The diff coverage is 70.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #654      +/-   ##
===========================================
- Coverage    92.90%   92.60%   -0.30%     
===========================================
  Files          318      320       +2     
  Lines        22360    22445      +85     
===========================================
+ Hits         20773    20785      +12     
- Misses        1587     1660      +73     
Impacted Files Coverage Δ
core/device_hooks/cuda_hooks.cpp 30.61% <0.00%> (-24.95%) ⬇️
core/device_hooks/hip_hooks.cpp 44.11% <0.00%> (-11.44%) ⬇️
core/devices/cuda/executor.cpp 25.00% <0.00%> (-25.00%) ⬇️
core/devices/hip/executor.cpp 14.28% <0.00%> (-19.05%) ⬇️
include/ginkgo/core/base/exception.hpp 93.42% <0.00%> (-6.58%) ⬇️
include/ginkgo/core/base/exception_helpers.hpp 90.90% <ø> (ø)
include/ginkgo/core/base/types.hpp 92.59% <ø> (ø)
include/ginkgo/core/base/executor.hpp 68.18% <25.64%> (-22.56%) ⬇️
core/devices/omp/executor.cpp 77.77% <50.00%> (-22.23%) ⬇️
core/base/memory_space.cpp 83.33% <83.33%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bfbc1d...48c9e61. Read the comment docs.

@pratikvn
Copy link
Member Author

pratikvn commented Apr 9, 2021

To reduce the number of PR's, I will close this for now as we dont want really long standing PR's anyway. We can open another one later maybe after 2.0

@pratikvn pratikvn closed this Apr 9, 2021
@pratikvn pratikvn deleted the memory-space-abstraction branch April 9, 2021 08:55
@pratikvn pratikvn restored the memory-space-abstraction branch April 9, 2021 08:57
@pratikvn pratikvn deleted the memory-space-abstraction branch April 9, 2021 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:WIP This PR is a work in progress. Not ready for review. is:affects-performance This is related to something which affects performance. is:experimental This is an experimental feature/PR/issue/module. is:help-wanted Need ideas on how to solve this. is:interface-breaking This change may break interface compatibility. Can only be done through a major release. mod:all This touches all Ginkgo modules.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants