ARROW-14269: [C++] Consolidate utf8 benchmark #11376
Closed
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
|
|
cyb70289
commented
Oct 11, 2021
| const uint8_t* data = reinterpret_cast<const uint8_t*>(str.data()); | ||
| const size_t length = str.size(); | ||
|
|
||
| return ValidateUTF8(data, length); | ||
| } | ||
|
|
||
| inline bool ValidateAsciiSw(const uint8_t* data, int64_t len) { | ||
| static inline bool ValidateAsciiSw(const uint8_t* data, int64_t len) { |
There was a problem hiding this comment.
Original code unrolls loop manually expecting to make better use of cpu pipeline. But it prevents the compiler to do better optimization leveraging auto vectorization.
Actually, this simple code performs same as simd code, if build with clang.
But gcc has big regression. So I left the simd code untouched.
- clang, simd no better than naive code
https://quick-bench.com/q/R5S9gDfyCzxs4pyQO3rLszMLCBI - gcc, simd is faster
https://quick-bench.com/q/Xes5_3-CGjJbYNikY0E0BWdfVTo
|
NOTE: below results are compared against the non-inlined benchmark (1st commit in this pr), not against master branch.
clang-10, Intel gold 5218 gcc-9, Intel gold 5218 Arm Neoverse N1, clang-10 Arm Neoverse N1, gcc-9 |
cyb70289
commented
Oct 11, 2021
Comment on lines
+27
to
+28
| // Do not benchmark inlined functions directly inside the benchmark loop | ||
| static ARROW_NOINLINE bool ValidateUTF8NoInline(const uint8_t* data, int64_t size) { |
There was a problem hiding this comment.
Without this noinline wrap, benchmarked function is expanded in the benchmark loop.
It causes strange behaviour. E.g., an obviously irrelevant change may bring big variance to the benchmark result.
|
@ursabot please benchmark |
pitrou
approved these changes
Oct 11, 2021
|
Benchmark runs are scheduled for baseline = 39875f1 and contender = a06c34f. Results will be available as each benchmark for each run completes. |
|
There is no important regression, so this can be merged IMHO. |
Don't benchmark inlined functions directly inside the benchmark loop. Instead, wrap them with non-inlined functions. This patch also improves performance of validating short ascii strings.
|
Rebased to fix merge conflict. |
|
Benchmark runs are scheduled for baseline = ca18e8a and contender = b01a30a. b01a30a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
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.
Don't benchmark inlined functions directly inside the benchmark loop.
Instead, wrap them with non-inlined functions.
This patch also improves performance of validating short ascii strings.