Force inlining to improve AST performance #9530
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
…f the expression_evaluator to improve null performance.
vyasr
added
3 - Ready for Review
|
Out of morbid curiosity, I tested a serial compilation of libcudf without ccache. After: And here are benchmarks (note the differences for nullable data): BenchmarksBefore: After |
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9530 +/- ##
================================================
- Coverage 10.79% 10.67% -0.12%
================================================
Files 116 117 +1
Lines 18869 19884 +1015
================================================
+ Hits 2036 2123 +87
- Misses 16833 17761 +928
Continue to review full report at Codecov.
|
jrhemstad
reviewed
Oct 27, 2021
jrhemstad
reviewed
Oct 30, 2021
bdice
reviewed
Nov 1, 2021
|
@jrhemstad @bdice do we want to keep this PR very limited in scope, or should I should modify all places in libcudf that use |
There are a decent handful of existing uses of |
jrhemstad
reviewed
Nov 3, 2021
jrhemstad
approved these changes
Nov 12, 2021
|
@gpucibot merge |
rapids-bot bot
pushed a commit
that referenced
this pull request
Jan 10, 2022
This PR is a follow-up to #9530 to standardize the names of the macros used for the `__host__ __device__` attributes. Aliases for `__device__` and combinations with inlining have been removed, and the only remaining macro is `CUDF_HOST_DEVICE` which is `__host__ __device__` in device code and empty in host code. See #9530 (comment) for more discussion. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Vukasin Milovanovic (https://github.com/vuule) - Jake Hemstad (https://github.com/jrhemstad) - Bradley Dice (https://github.com/bdice) - Mark Harris (https://github.com/harrism) URL: #9797
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 is a minimal set of changes that appear to be required to maximize inlining of code involved in AST-based expression evaluation. Specifically, applying the
__forceinline__qualifier to the type dispatcher results in a significant performance improvement when nullable data is passed through the AST evaluator. I observe roughly a 2x performance improvement with a negligible increase in compilation time. Note that the specific improvements appear to be heavily dependent on the architecture being tested on. Interestingly, removing the__forceinline__on theevaluatemethods results in performance regressing back to its original values despite them being defined inline, an unexpected result.