[DataLoader] Move loop content into a function to ensure we don't preserve anything #83595
Conversation
🔗 Helpful links
✅ No Failures (0 Pending)As of commit 93cd751 (more details on the Dr. CI page): Expand to see more💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
NivekT
changed the title
NivekT
added
release notes: dataloader
There was a problem hiding this comment.
I think this is faster (not such by how much) and should not cause any issue. I want to benchmark this to see if there is any noticeable difference but haven't gotten to it yet.
@VitalyFedyunin @ejguan, can you confirm this doesn't cause any problem?
| break | ||
| except queue.Full: | ||
| continue | ||
| del r # save memory |
There was a problem hiding this comment.
I feel like it would be simpler if we just do r = None at the end if we want to move the Tensor out of scope. WDYT?
But, the original problem that GIL is blocked still persist when gc tries to clean up the Tensor.
There was a problem hiding this comment.
I feel like it would be simpler if we just do r = None at the end if we want to move the Tensor out of scope. WDYT?
The problem is that doing r=None here only clear the tuple itself. data is still a local variable that remain alive. So this line is doing pretty much nothing today.
We could do del r, data to solve this. But moving it into the function makes it more future-proof as all local state will be properly removed.
But, the original problem that GIL is blocked still persist when gc tries to clean up the Tensor.
Not sure how this is linked?
If you're talking about expensive unmap that happen on these objects, #83623 should fix that.
There was a problem hiding this comment.
If you're talking about expensive unmap that happen on these objects, #83623 should fix that.
Thanks for pointing it out. It looks good.
The problem is that doing
r=Nonehere only clear the tuple itself.datais still a local variable that remain alive. So this line is doing pretty much nothing today. We could dodel r, datato solve this. But moving it into the function makes it more future-proof as all local state will be properly removed.
Oh, yeah. Tensor is also referenced by data. I agree on the idea of future proof. And, do you mind moving this function out of while not done_event.is_set():? Otherwise, I believe each iteration would create a new function.
There was a problem hiding this comment.
Yes I left it there on purpose but without any strong reason.
Re-creating it means that there is no captured variable kept alive by the function.
But that shouldn't happen here indeed. moving it out!
albanD
force-pushed
the
pin_memory_dataloader_nit
branch
from
0f0dde1 to
93cd751
Compare
|
@pytorchbot merge -g |
|
@pytorchbot successfully started a merge job. Check the current status here. |
…serve anything (#83595) (#83595) Summary: Can lead to CPU memory saving as we don't hold onto the pin memory buffer as long as we used to. Pull Request resolved: #83595 Approved by: https://github.com/ejguan, https://github.com/NivekT Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/38348362608a47371c65d7fd52db138b4c6a5d65 Reviewed By: atalman Differential Revision: D38852489 Pulled By: albanD fbshipit-source-id: 13021b949a34b64ffb9835992b77ef82fcbb0e85
Can lead to CPU memory saving as we don't hold onto the pin memory buffer as long as we used to.