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

Fix iterator-related memory issues #620

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Oct 27, 2023

MERGE after #618

Fix memory corruption issue in ThreadBatchDataLoader

Fix #592

  • Return a shared pointer of CuImageIterator, taking ownership of CuImageIterator instance, instead of returning a copy of CuImageIterator
  • Do not create new instance for CuImageIterator when __iter__ is called. Instead, return self.
  • Wait until all threads are finished in ThreadBatchDataLoader destructor

Fix memory leak in ThreadBatchDataLoader

Fix #598

  • Throw StopIteration exception early
  • Do not set nullptr for raster_data_ in ThreadBatchDataLoader::next_data()
    • Setting nullptr prevents freeing memory in the destructor of ThreadBatchDataLoader

Fix fatal errors for cache object

  • GIL is not acquired when using Python object.

Add iterator-related test cases for memory issue

@gigony gigony requested review from a team as code owners October 27, 2023 09:13
@gigony gigony self-assigned this Oct 27, 2023
@gigony gigony added bug Something isn't working non-breaking Introduces a non-breaking change labels Oct 27, 2023
@gigony gigony force-pushed the fix_iterator_issues branch 2 times, most recently from 632588d to 293f104 Compare October 27, 2023 09:44
@gigony gigony requested a review from grlee77 October 27, 2023 09:53
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks, Gigon. It looks good to me. I left an optional suggestion regarding comments on the test cases

@jakirkham jakirkham added this to the v23.12.00 milestone Oct 27, 2023
@jakirkham
Copy link
Member

Have merged PR: #618

Looks like there are some conflicts

Signed-off-by: Gigon Bae <gbae@nvidia.com>
GIL is not acquired when using Python object.

Signed-off-by: Gigon Bae <gbae@nvidia.com>
- Return a shared pointer of CuImageIterator, taking ownership of
  CuImageIterator instance, instead of returning a copy of
  CuImageIterator
- Do not create new instance for CuImageIterator when __iter__ is
  called. Instead, return self.
- Wait until all threads are finished in ThreadBatchDataLoader
  destructor

Signed-off-by: Gigon Bae <gbae@nvidia.com>
- Throw StopIteration exception early
- Do not set nullptr for 'raster_data_' in
  ThreadBatchDataLoader::next_data()
  - Setting nullptr prevents freeing memory in the destructor
    of ThreadBatchDataLoader

Signed-off-by: Gigon Bae <gbae@nvidia.com>
@jakirkham
Copy link
Member

/merge

@gigony
Copy link
Contributor Author

gigony commented Oct 30, 2023

I am seeing an error message regarding cpu memory usage:
https://github.com/rapidsai/cucim/actions/runs/6698898284/job/18202421711?pr=620
image

It is likely that it is intermittent issue (may need to increase the threshold > 1273671680 - 1223131136 = 50540544).

@rapids-bot rapids-bot bot merged commit f95e100 into rapidsai:branch-23.12 Oct 30, 2023
27 checks passed
@jakirkham
Copy link
Member

Thanks Gigon! 🙏

Should we file a follow-up issue or PR?

rapids-bot bot pushed a commit that referenced this pull request Oct 31, 2023
…est (#623)

There is a case where memory usage check is not stable enough:
#620 (comment)

Instead of choosing index 1 and 4 (among 5 samples), choose 5 and 9 (among 10 samples) for memory leak check.

Authors:
  - Gigon Bae (https://github.com/gigony)

Approvers:
  - Gregory Lee (https://github.com/grlee77)

URL: #623
@jakirkham
Copy link
Member

It is likely that it is intermittent issue (may need to increase the threshold > 1273671680 - 1223131136 = 50540544).

Filed as issue: #625

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
Status: Done
3 participants