-
Notifications
You must be signed in to change notification settings - Fork 991
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
refactor: Reduce string allocations in Expr::display_name (use write instead of format!) #10454
refactor: Reduce string allocations in Expr::display_name (use write instead of format!) #10454
Conversation
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 it is a great idea to speed up planning by avoiding string allocations in Expr::display_name
@erratic-pattern -- thank you 🙏
I left some suggestions for small additional improvements, but I also think this PR could be merged as is.
I am going to run my planing benchmarks too to see if we can measure any difference here
datafusion/expr/src/expr.rs
Outdated
fn write_function_name<'a>( | ||
w: &'a mut (dyn Write + 'a), |
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.
Rather than using dynamic dispatch I think you can make this just a normal generic function and make the code simpler and likely more performant
Something like
fn write_function_name<W: Write>(
w: &mut W,
I actually tried it locally and it seems to work well. Here is a PR erratic-pattern#1 to this branch for your consideration
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.
Rather than using dynamic dispatch I think you can make this just a normal generic function and make the code simpler and likely more performant
Something like
fn write_function_name<W: Write>( w: &mut W,I actually tried it locally and it seems to work well. Here is a PR erratic-pattern#1 to this branch for your consideration
I didn't find a meaningful difference in performance here vs monomorphic types (I tried &mut String
explicitly in my testing, which is similar code to the generic here). In fact Formatter
in the standard library also has an internal dyn Write
, though it has an actual need for ad-hoc polymorphism whereas we don't. I am guessing it gets optimized in most cases, but it certainly wouldn't hurt to make it explicitly generic so I agree with this change.
datafusion/expr/src/expr.rs
Outdated
@@ -1693,10 +1708,9 @@ pub(crate) fn create_name(e: &Expr) -> Result<String> { | |||
if let Some(char) = escape_char { | |||
format!("CHAR '{char}'") | |||
} else { | |||
"".to_string() | |||
"".to_owned() |
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 you could avoid this allocation (and the format!
above it).
datafusion/expr/src/expr.rs
Outdated
@@ -1717,111 +1732,118 @@ pub(crate) fn create_name(e: &Expr) -> Result<String> { | |||
if let Some(char) = escape_char { | |||
format!("CHAR '{char}'") | |||
} else { | |||
"".to_string() | |||
"".to_owned() |
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.
same comment as above -- let's get rid of all the extra allocations
Thanks @erratic-pattern -- I took the liberty of merging the branch up from main to resolve a merge conflict as well |
63a1656
to
1d8ecd1
Compare
Wow -- according to my benchmarks this change makes a non trivial difference in performance. We just keep driving tese numbers down
|
🚀 |
Nice! Those results look a lot better than what I found on my laptop. Very hard to get consistent benchmark results on a personal computer when there's so much process scheduling noise |
Yeah, I have a gcp VM running on which I run the benchmarks |
Just a small refactor that should, in theory, reduce string allocations and thus benefit concurrent throughput by reducing allocator lock contention. However, when running concurrency benchmarks on my M3 Max I only saw a minor improvement in InfluxDB (about 40 additional queries per second with 3 threads, but no change in overall curve as # of threads increases).
I don't have any strong opinion on whether or not this should be merged in, but I might as well submit it as a PR.