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

Make tables spillable by default #8264

Merged
merged 11 commits into from
May 23, 2023

Conversation

abellina
Copy link
Collaborator

@abellina abellina commented May 9, 2023

Closes #7672

This depends on rapidsai/cudf#13260 (rapidsai/cudf#13180 overarching issue)

The idea behind this is to be able to add a Table as an object into the spillable store. The Table will not be made contiguous until that is needed at spill time (to host memory), which is done using chunked_pack. Once it has gone through chunked_pack the table will be reconstituted as a regular RapidsDeviceMemoryBuffer with a single contiguous allocation.

It is draft because I still need to clean it up some and I need to add unit tests specific to this scenario. I also did a bunch of cleanup today and I have not run the latest, so I could have bugs here. But nevertheless, I think it is worth starting to take a look at it.

It won't pass the build because the cuDF PRs are not in yet, and I have one test failure that I know of.

Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Mostly nits and comment requests. But I want to spend some more time looking at the tests.

@sameerz sameerz added reliability Features to improve reliability or bugs that severly impact the reliability of the plugin performance A performance related task/issue labels May 11, 2023
@jbrennan333
Copy link
Contributor

Overall this is excellent work @abellina! I will likely do another pass, but so far my comments all overlap with those from @revans2.

@abellina
Copy link
Collaborator Author

Updating the full NDS benchmark with rapidsai/cudf#13260 + rapidsai/cudf#13278 + #8264 (this), I am seeing a ~4% improvement vs the last nightly.

I looked at one of the queries (q95) which becomes ~14% faster. Before the change ~7% of kernel time was spent in this function, whereas now it's less than 0.1%.

Name = benchmark
Means = 417757.4, 401496.8
Time diff = 16260.600000000035
Speedup = 1.0404999491901306
T-Test (test statistic, p value, df) = 9.948692331384382, 8.820392981431996e-06, 8.0
T-Test Confidence Interval = 12491.56085504279, 20029.63914495728
ALERT: significant change has been detected (p-value < 0.05)
ALERT: improvement in performance has been observed

@abellina
Copy link
Collaborator Author

working on review comments locally and will push but this won't be merge-able until Monday.

revans2
revans2 previously approved these changes May 22, 2023
@abellina abellina marked this pull request as ready for review May 22, 2023 18:44
@abellina
Copy link
Collaborator Author

build

revans2
revans2 previously approved these changes May 22, 2023
@abellina
Copy link
Collaborator Author

build

@abellina
Copy link
Collaborator Author

build

@abellina
Copy link
Collaborator Author

Ok, @revans2 the reason for c3c2179 was a change I had made in the test locally and forgotten to undo it. It should be a single close because the code throws a retry and split within a retry (this tests the specific exception), and so every batch that was open was told to close. I do not remember why I had changed this locally, but overall it removes the whole file from the unit test changes.

@abellina abellina merged commit 6bd9768 into NVIDIA:branch-23.06 May 23, 2023
@abellina abellina deleted the spillable_tables branch May 23, 2023 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance A performance related task/issue reliability Features to improve reliability or bugs that severly impact the reliability of the plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make all buffers/columnar batches spillable by default
4 participants