-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Feature] Import PyTorch's pin_memory()
method for DGL graph structure
#5366
Conversation
To trigger regression tests:
|
This comment was marked as outdated.
This comment was marked as outdated.
@jermainewang Pls help add other reviewers. Thanks. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Hi Chang, sorry being picky on the code quality. Overall the PR looks very good to me, just some small tweak to make it shiny. |
No worries @frozenbugs I am always happy to iterate and improve in every aspect. Thank you for taking the time and review this PR. I will address them all tomorrow. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for some comments issues.
2be8547
to
bd4ff7a
Compare
Sorry for the delay. Ready for the next round @frozenbugs @BarclayII @yaox12 |
ebcfa02
to
0353012
Compare
0353012
to
5249390
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, overall this is a giant PR which we should try to avoid if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good. Thanks a lot!
Description
This PR imports pytorch's
CachingHostAllocator
[ref] and its associated functions (likeCachingHostAllocator_recordEvent
andCachingHostAllocator_emptyCache
) to DGL'stensoradaptor
. It enhances/enriches DGL's native memory pinning strategy (cudaHostRegister
), by using backend's pinned memory pool (viacudaHostAlloc
) to avoid overhead fromcudaHostRegister
and frequent memory allocations/deallocations.While
cudaHostRegister
(DGLGraph.pin_memory_
) is more suitable to pin large data (like entire feature/graph) ONCE for future frequent GPU accesses (no memory allocation involved but with the compromising of speed), pytorch's pinned memory pool (DGLGraph(Index).pin_memory
) is better for objects (small memory blocks) that could be created/destroyed often (like DGLGraph/feature for batches).Based on this PR, it would be easier to implement
pin_memory()
method for DGLGraph object, and let PyTorch's dataloader pick it up whenpin_memory=True
to enable async H2D copying of (sub)graph structure (or feature if needed). Complete dataloader support should be included in a separate PR.Checklist
Please feel free to remove inapplicable items for your PR.
Changes
C++ (DGL core):
getCachingHostAllocator
,CachingHostAllocator_recordEvent
andCachingHostAllocator_emptyCache
to completely support pytorch's cachingHostAllocator.pin_memory*
methods to all necessary classes(ndarry/csr/coo/unitgraph/heterograph).RecordedCopyDataFromTo
to fuse data copy (H2D) and followingCachingHostAllocator_recordEvent
to manage the (pinned)resource consistently withbackend PyTorch.cudaHostRegister
to release resources/prepare enough page-locked memory needed bycudaHostRegister
Python:
pin_memory()
forheterograph_index
To-dos
(maybe need separate PRs)
Python:
pin_memory()
forheterograph
or other equivalents to integrate with dataloader.