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

[JNI] Adds an EventHandler to Java MemoryBuffer to be invoked on close #12125

Merged
merged 2 commits into from
Nov 11, 2022

Conversation

abellina
Copy link
Contributor

This PR adds an EventHandler to MemoryBuffer with a single method onClosed. This is invoked during the close call, but after the refCount has been updated.

I am also making getRefCount public in this PR. Spill code in the RAPIDS Accelerator for Spark could likely assert/require that refCount==1 when taking in a new buffer to be spillable. This last change is a nice to have.

@abellina abellina added feature request New feature or request Java Affects Java cuDF API. Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change labels Nov 10, 2022
@abellina abellina requested a review from a team as a code owner November 10, 2022 19:09
@abellina abellina changed the title Adds an EventHandler to Java MemoryBuffer to be invoked on close [JNI] Adds an EventHandler to Java MemoryBuffer to be invoked on close Nov 10, 2022
Copy link
Contributor

@jbrennan333 jbrennan333 left a comment

Choose a reason for hiding this comment

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

+1 lgtm

@gerashegalov
Copy link
Contributor

Should we start exploring Java Instrumentation API such that we can keep auxiliary monitoring code out the production codebase similar to CUPTI

@abellina
Copy link
Contributor Author

Should we start exploring Java Instrumentation API such that we can keep auxiliary monitoring code out the production codebase similar to CUPTI

In this scenario this is production code. We expect to use this in the spill framework.

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 87.47% // Head: 88.11% // Increases project coverage by +0.63% 🎉

Coverage data is based on head (31bc4d5) compared to base (f817d96).
Patch has no changes to coverable lines.

❗ Current head 31bc4d5 differs from pull request most recent head 757b317. Consider uploading reports for the commit 757b317 to get more accurate results

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.12   #12125      +/-   ##
================================================
+ Coverage         87.47%   88.11%   +0.63%     
================================================
  Files               133      135       +2     
  Lines             21826    22100     +274     
================================================
+ Hits              19093    19473     +380     
+ Misses             2733     2627     -106     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/interval.py 85.45% <0.00%> (-9.10%) ⬇️
python/cudf/cudf/io/text.py 91.66% <0.00%> (-8.34%) ⬇️
python/cudf/cudf/core/_base_index.py 81.28% <0.00%> (-4.27%) ⬇️
python/cudf/cudf/io/json.py 92.06% <0.00%> (-2.68%) ⬇️
python/cudf/cudf/utils/utils.py 89.91% <0.00%> (-0.69%) ⬇️
python/cudf/cudf/core/column/timedelta.py 90.17% <0.00%> (-0.58%) ⬇️
python/cudf/cudf/core/column/datetime.py 89.21% <0.00%> (-0.51%) ⬇️
python/cudf/cudf/core/column/column.py 87.96% <0.00%> (-0.46%) ⬇️
python/dask_cudf/dask_cudf/core.py 73.72% <0.00%> (-0.41%) ⬇️
python/cudf/cudf/io/parquet.py 90.45% <0.00%> (-0.39%) ⬇️
... and 43 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@abellina
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8668752 into rapidsai:branch-22.12 Nov 11, 2022
@abellina abellina deleted the spill/memory_buffer_cb branch November 11, 2022 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request Java Affects Java cuDF API. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants