-
Notifications
You must be signed in to change notification settings - Fork 2k
perf: Optimize scalar fast path of to_hex function #20112
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
Conversation
|
might be related #20044 |
|
|
||
| #[inline] | ||
| fn to_hex_scalar<T: ToHex>(value: T) -> String { | ||
| let mut hex_buffer = [0u8; 16]; |
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.
we prob can make it reusable buffer?
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.
Thanks for the feedback. My initial thinking was the scalar fast path buffer is a fixed 16‑byte stack array, so it’s already very cheap and not heap-allocating. The array path is where buffer reuse matters and we already reuse a single buffer across the whole batch there. What do you think, should we still go ahead with the buffer reuse change?
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.
This function should be called only once per invocation so I don't think we need to change this to reuse a buffer. Looks like the array path already has a reusable buffer too
comphead
left a comment
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.
Thanks @kumarUjjawal I think this is great, however you can check if the reusable buffer can bring up more benefits
|
|
||
| #[inline] | ||
| fn to_hex_scalar<T: ToHex>(value: T) -> String { | ||
| let mut hex_buffer = [0u8; 16]; |
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.
This function should be called only once per invocation so I don't think we need to change this to reuse a buffer. Looks like the array path already has a reusable buffer too
comphead
left a comment
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.
Thanks @kumarUjjawal and @Jefffrey for the review
Which issue does this PR close?
Rationale for this change
to_hex(used by benchmark itemshex_int/hex_long) previously routed evaluation throughmake_scalar_function(..., vec![]), which converts scalar inputs into size‑1 arrays before execution. This adds avoidable overhead for constant folding / scalar evaluation.What changes are included in this PR?
Int8/16/32/64andUInt8/16/32/64make_scalar_function(..., vec![])usageto_hex/scalar_i32to_hex/scalar_i64Are these changes tested?
Yes
Are there any user-facing changes?
No