[REVIEW] Feature/optimize accessor copy #7660
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e the columns in the accessor.
vyasr
added
non-breaking
shwina
approved these changes
Mar 19, 2021
kkraus14
approved these changes
Mar 23, 2021
kkraus14
added
5 - Ready to Merge
shwina
reviewed
Mar 23, 2021
shwina
reviewed
Mar 23, 2021
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #7660 +/- ##
===============================================
+ Coverage 81.86% 82.49% +0.62%
===============================================
Files 101 101
Lines 16884 17416 +532
===============================================
+ Hits 13822 14367 +545
+ Misses 3062 3049 -13
Continue to review full report at Codecov.
|
|
@gpucibot merge |
rapids-bot bot
pushed a commit
that referenced
this pull request
Mar 24, 2021
This PR introduces various small optimizations that should generally improve various common Python overhead. See #7454 (comment) for the motivation behind these optimizations and some benchmarks. Merge after: #7660 Summary: * Adds a way to initialize a ColumnAccessor (_init_unsafe) without validating its input. This is useful when converting a `cudf::table` to a `Frame`, where we're guaranteed the columns are well formed * Improved (faster) `is_numerical_dtype` * Prioritize check for numeric dtypes in `astype()` and `build_column()`. Numeric types are presumably more common, and we can avoid expensive checks for other dtypes this way. Authors: - Ashwin Srinath (@shwina) Approvers: - Keith Kraus (@kkraus14) URL: #7686
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes a number of changes to resolve #7646:
ColumnAccessorhas been converted to adict. The conversion from anOrderedDictsubclass to a rawdictis safe because, as of Python 3.7, dictionary ordering is guaranteed by the CPython standard.OrderedColumnDictclass has instead been moved to a new method that is called inColumnAccessor.set_by_label. This ensures that the validation only happens once, and dramatically speeds up most dict operations since they can now rely on the built-in C implementation of__setitem__rather than Python overrides. Furthermore, this obviates the safety concerns raised in Unsafe usage of OrderedDict inheritance #7646 since we are no longer overriding these methods.ColumnAccessorhas been carefully optimized to handle validation with minimal overhead. Previously, this was handled by_data.__setitem__.ColumnAccessoris stored the first time a column is added so that it doesn't have to be recomputed.validatehas been added to_set_by_label. This parameter is currently unused, but can be used in future refactorings to further speed up the code by bypassing validation steps when we are internally passing columns that we know to be safe.The performance implications of these changes for shallow copying
DataFrameobjects are shown below. Specifically, I'm benchmarkingdf.copy(deep=False)for different numbers of columns and column sizes. As expected, these changes are largely invariant to the size of the columns and scale with the number of columns. The scaling isn't perfectly linear because of the constant overhead of copying theColumnAccessoritself, but the speedup plateaus around 5x for >200 columns (which probably isn't too realistic anyway). Exactly how these speedups will propagate to operations like joins remains to be seen, but this should help since some crude benchmarking I did showed that copying can take up to 15-20% of the total time of a join depending on the data size. @shwina has better benchmarks there and I'll work with him on future changes. Here's the benchmarking notebook I used. Note that since Github doesn't support uploading notebooks I've changed the extension to txt, but it will work find to just rename it to a .ipynb file.