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.

Sign up for GitHub

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

Jump to bottom

Replace whole-buffer slicing with direct refcounting #894

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Replace whole-buffer slicing with direct refcounting #894

merged 1 commit into from
Sep 30, 2020

Conversation

jlowe
Copy link
Member

@jlowe jlowe commented Sep 30, 2020

Fixes #585.

This changes occurrences where we were slicing the range of an entire buffer with a more direct increment of the reference count and reuse of the same buffer object. This cuts down on unnecessary object creation.

@jlowe
Replace whole-buffer slicing with incRefCount
c17b33b 
Signed-off-by: Jason Lowe <jlowe@nvidia.com>
@jlowe jlowe added this to the Sep 28 - Oct 9 milestone Sep 30, 2020
@jlowe jlowe self-assigned this Sep 30, 2020
@jlowe
Copy link
Member Author

jlowe commented Sep 30, 2020

build

@revans2
Copy link
Collaborator

revans2 commented Sep 30, 2020

Did you run any benchmarks to see if there was a change?

@jlowe
Copy link
Member Author

jlowe commented Sep 30, 2020

Did you run any benchmarks to see if there was a change?

Nothing rigorous like a microbenchmark. I did run TpcxbbLikeSpark.Q21Like on scale factor 100GB data a couple of times on my desktop, mostly to verify I didn't see any leak issues, and I did get times that were the best I've seen on it today. However the time deltas from the previous runs were relatively small (around 250ms to 500ms on a query whose wallclock time is around 30 seconds) and could easily be in the noise.

My main motivation for implementing this was to cut down on the number of DeviceMemoryBuffer instances floating around in the average heap dump, making it a bit easier to track down what's going on. It should also be theoretically faster, but as you mentioned offline, probably not a significant win there.

@jlowe
Copy link
Member Author

jlowe commented Sep 30, 2020

build

@jlowe
Copy link
Member Author

jlowe commented Sep 30, 2020

#724 is starting to become quite annoying, seeing it fairly often in premerge CI.

@jlowe jlowe merged commit 1e80aa3 into NVIDIA:branch-0.3 Sep 30, 2020
@sameerz sameerz added the feature request New feature or request label Oct 14, 2020
@jlowe jlowe deleted the slice-to-incref branch October 28, 2020 16:42
sperlingxx pushed a commit to sperlingxx/spark-rapids that referenced this pull request Nov 20, 2020
@jlowe @sperlingxx
Replace whole-buffer slicing with incRefCount (NVIDIA#894)
51e7b6e 
Signed-off-by: Jason Lowe <jlowe@nvidia.com>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@jlowe
Replace whole-buffer slicing with incRefCount (NVIDIA#894)
7be7067 
Signed-off-by: Jason Lowe <jlowe@nvidia.com>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@jlowe
Replace whole-buffer slicing with incRefCount (NVIDIA#894)
b24ffe7 
Signed-off-by: Jason Lowe <jlowe@nvidia.com>
tgravescs pushed a commit to tgravescs/spark-rapids that referenced this pull request Nov 30, 2023
@nvauto
Update submodule cudf to f7d434d6b633b3d5c68cb8f11063d193c504eee9 (NV…
a9e1715 
…IDIA#894)

Signed-off-by: spark-rapids automation <70000568+nvauto@users.noreply.github.com>

Signed-off-by: spark-rapids automation <70000568+nvauto@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revans2 revans2 revans2 approved these changes

Assignees

@jlowe jlowe

Labels
feature request New feature or request
Projects
None yet
Milestone
Sep 28 - Oct 9
Development

Successfully merging this pull request may close these issues.

[FEA] Replace whole-buffer slicing with reference count increment
3 participants
@jlowe @revans2 @sameerz