Uh oh!
There was an error while loading. Please reload this page.
perf: Use Arc<str> instead of Cow<&'a> in the analyzer - #9824
Merged
Conversation
comphead
commented
Mar 27, 2024
ContributorAuthor
@alamb do you run sql planner benchmark vs baseline? |
comphead
marked this pull request as draft
March 27, 2024 15:41
ContributorAuthor
With |
comphead
commented
Mar 28, 2024
ContributorAuthor
@alamb@jayzhan211 I can see some improvements for the |
jayzhan211
commented
Mar 28, 2024
Contributor
Is it possible to replace with |
ContributorAuthor
I'll try but imho I dont expect much diff as those strings are mostly immutable and should not be much diff between |
alamb
commented
Mar 28, 2024
Contributor
I agree there is not a lot of practical difference -- but I think the for |
alamb
commented
Mar 28, 2024
Contributor
I think we should proceed 🚀 What I think we should do is get version 37 released so we can proceed. |
comphead
commented
Mar 28, 2024
ContributorAuthor
yes, its 1 more level of indirection, I'll try to go with |
alamb
commented
Mar 28, 2024
Contributor
Most of the time, you can make an |
comphead
commented
Mar 29, 2024
ContributorAuthor
oops |
ContributorAuthor
with |
comphead
commented
Apr 1, 2024
ContributorAuthor
Latest @alamb@jayzhan211 which way you guys prefer? |
Arc<String> instead of Cow<&'a> in the analyzercomphead
marked this pull request as ready for review
April 2, 2024 01:28
Contributor
The technical difference between the two is:
So If you measure a perf difference, then this is probably because one (or more) of these points hold:
That said, since |
alamb
commented
Apr 2, 2024
Contributor
I prefer |
alamb
commented
Apr 2, 2024
Contributor
I am feeling DataFusion planning time is going to be very much improved in Version 38.0.0 🚀 -- I'll check this PR out later today. Very excited |
7 tasks
ContributorAuthor
Modified to |
comphead
commented
Apr 2, 2024
| } | ||
| pub type OwnedSchemaReference = SchemaReference<'static>; | ||
| pub type OwnedSchemaReference = SchemaReference; |
ContributorAuthor
There was a problem hiding this comment.
Gonna deprecate this type in following PR
comphead
commented
Apr 2, 2024
| @@ -246,24 +240,7 @@ impl<'a> TableReference<'a> { | |||
| /// Converts directly into an [`OwnedTableReference`] by cloning | |||
| /// the underlying data. | |||
| pub fn to_owned_reference(&self) -> OwnedTableReference { | |||
ContributorAuthor
There was a problem hiding this comment.
gonna deprecate this method in following PR
Arc<String> instead of Cow<&'a> in the analyzerArc<str> instead of Cow<&'a> in the analyzeralamb
approved these changes
Apr 2, 2024
alamb
left a comment
Contributor
There was a problem hiding this comment.
TLDR is I think this is awesome -- thank you @comphead . Our planning speed is going to be very sweet indeed.
Modified to Arc as discussed. The better performance benefit for Arc I think still explained by using String more often across the analyzer than str and the conversion is cheaper
Yeah, this is really strange. I think the issue is exactly as you describe, which is that using an Arc<String> can simply take the string that was allocated by
Latest Arc consistently improves 5-7%, in very good cases even 14%
I am sorry I think I missed this comment. In would like to change my vote and say Arc<String> though I realize you already changed the PR once so I think it would also be ok to merge this as is
Contributor
I am also running the benchmarks as well and I am going to try and make a test PR that tries using Update: testing with #9916 ps. it would be sweet to see what this does for the TPC-DS benchmarks #9907 |
comphead
commented
Apr 2, 2024
ContributorAuthor
I'll merge it as is, and will create another one with Arc again, so we can see if there is more benefits |
alamb
commented
Apr 2, 2024
Contributor
Sounds good -- we can also potentially switch to using |
alamb
commented
Apr 2, 2024
Contributor
Update it turns out that I got significant performance improvements by using |
comphead
commented
Apr 2, 2024
ContributorAuthor
Yes, my very first implementation was to get the |
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Which issue does this PR close?
Closes#9764
Rationale for this change
In the analyzer there are mostly immutable strings so we may want to use
Arc<str>which performs better thanCow<&'a str>for immutable stringsWhat changes are included in this PR?
Replace datatype
Are these changes tested?
yes
Are there any user-facing changes?