-
Notifications
You must be signed in to change notification settings - Fork 721
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
Speed up the function min_max_string
#1374
Conversation
Signed-off-by: remzi <13716567376yh@gmail.com>
Signed-off-by: remzi <13716567376yh@gmail.com>
BenchmarkBeforemin string 512 time: [1.4824 us 1.4914 us 1.5094 us]
change: [-2.4834% -0.7235% +0.5457%] (p = 0.44 > 0.05)
No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
2 (2.00%) high mild
11 (11.00%) high severe
min nulls string 512 time: [2.4456 us 2.4528 us 2.4610 us]
change: [-0.7299% -0.2597% +0.2043%] (p = 0.26 > 0.05)
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
7 (7.00%) high mild
2 (2.00%) high severe Aftermin string 512 time: [1.5539 us 1.5552 us 1.5565 us]
change: [-0.4238% -0.2174% -0.0106%] (p = 0.04 < 0.05)
Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) high mild
4 (4.00%) high severe
min nulls string 512 time: [1.8519 us 1.8536 us 1.8552 us]
change: [-1.0138% -0.7613% -0.4859%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
1 (1.00%) high mild
1 (1.00%) high severe |
Codecov Report
@@ Coverage Diff @@
## master #1374 +/- ##
==========================================
+ Coverage 82.99% 83.03% +0.04%
==========================================
Files 181 181
Lines 52933 53003 +70
==========================================
+ Hits 43932 44013 +81
+ Misses 9001 8990 -11
Continue to review full report at Codecov.
|
I have tested that if we replace |
Signed-off-by: remzi <13716567376yh@gmail.com>
n = item; | ||
} | ||
} | ||
array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to your benchmark result, this new way a bit slower than before for nulls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. You can see the benchmark "min nulls string 512". Time decreases from 2.4us to 1.8us. I this this is performance improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for the benchmark "min string 512 ", there is no big performance penalty introduced. (time from 1.49us to 1.55us)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could speed it up a bit more if we could iterate fast on the non-null values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me try the array.value_unchecked()
10% extra speed up Signed-off-by: remzi <13716567376yh@gmail.com>
min string 512 time: [1.3687 us 1.3691 us 1.3696 us]
change: [-7.7279% -7.5833% -7.4242%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
1 (1.00%) high mild
8 (8.00%) high severe
min nulls string 512 time: [1.6630 us 1.6647 us 1.6665 us]
change: [-33.478% -33.362% -33.244%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe Using |
Signed-off-by: remzi <13716567376yh@gmail.com>
min_max_string
min
and max
aggregation kernls (optimize min_max_string
)
min
and max
aggregation kernls (optimize min_max_string
)min
and max
aggregation kernels without nulls
min
and max
aggregation kernels without nullsmin_max_string
Thanks @HaoYang670 and @Dandandan |
Which issue does this PR close?
Closes #1373 .
Rationale for this change
What changes are included in this PR?
null_count > 0
. See benchmark result.benchmark
Are there any user-facing changes?
No