-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
improve performance for materializing a string from a vector of Char #29551
Conversation
@nanosoldier |
@ararslan, any idea how nanosoldier is doing? |
Hm, there were no errors in the log or anything, it just seemed to be stuck. I've restarted the server, so let's just try again. @nanosoldier |
base/strings/io.jl
Outdated
@@ -576,6 +576,19 @@ function unindent(str::AbstractString, indent::Int; tabwidth=8) | |||
String(take!(buf)) | |||
end | |||
|
|||
function String(a::Vector{Char}) |
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.
Maybe the signature could be a::AbstractVector{Char}
?
base/strings/substring.jl
Outdated
@@ -145,6 +145,28 @@ end | |||
string(a::String) = String(a) | |||
string(a::SubString{String}) = String(a) | |||
|
|||
@inline function __string!(out, c::Char, offs::Integer) |
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.
maybe name it __unsafe_string!
to highlight the fact that it does not perform bounds checking?
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
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.
Looks like a good optimization, but I disagree that this should be backported.
fine |
Can you elaborate on why? |
Why? |
Before
After
The unrolling of the loop to write the char made a significant difference here (but is not where the major performance benefit comes from).
cc @bkamins