Skip to content
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

specialize VersionNumber constructors less aggressively #54386

Merged
merged 1 commit into from
May 7, 2024

Conversation

topolarity
Copy link
Member

There's no need to specialize this code for every set of Tuple types that it might see, especially when the primary callers don't know their Tuple type any more precisely than this anyway.

Eliminates a dynamic dispatch from tryparse(VersionNumber, "...")

There's no need to specialize this code for every set of Tuple types
that it might see, especially when the primary callers don't know their
Tuple type any more precisely than this anyway.

Co-authored-by: Gabriel Baraldi <baraldigabriel@gmail.com>
@topolarity topolarity changed the title Specialize VersionNumber constructors less aggressively specialize VersionNumber constructors less aggressively May 6, 2024
base/version.jl Outdated
pre::Tuple{Vararg{Union{Integer,AbstractString}}} = (),
bld::Tuple{Vararg{Union{Integer,AbstractString}}} = ()) =
@nospecialize(pre::Tuple{Vararg{Union{Integer,AbstractString}}} = ()),
@nospecialize(bld::Tuple{Vararg{Union{Integer,AbstractString}}} = ())) =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, this is probably not the right change...

We'd really like to specialize on the Vararg parameters here, but not on the length of the tuple.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I'm going to back out of this part of these changes, until we have a solution for: #54390

Thankfully, the other call-site should be fine to de-specialize since it's a small, concrete union.

@topolarity topolarity force-pushed the ct/despecialize-VersionNumber-constructors branch from 7273567 to fb130fa Compare May 7, 2024 11:45
@aviatesk
Copy link
Member

aviatesk commented May 7, 2024

If it proves ineffective to generate code based on the tuple lengths for this case, should we probably refactor the whole code to use Vector{Union{UInt64,String}} for representing the pre/pld information?

@topolarity
Copy link
Member Author

I believe these are considered public constructors at this point, so I'm not sure we have the freedom to do that

@jakobnissen
Copy link
Contributor

jakobnissen commented May 7, 2024

This was made public in #50179 where it was (apparently) explicitly decided that it should stay a Tuple

@mcabbott
Copy link
Contributor

mcabbott commented May 7, 2024

FWIW, a quick attempt to time this... maybe you have a better measurement in mind, as part of something larger?

julia> @btime tryparse(VersionNumber, $("2.0.1-rc1"))  # master
  min 915.300 ns, mean 1.011 μs (17 allocations, 744 bytes)
v"2.0.1-rc1"

julia> @btime tryparse(VersionNumber, $("2.0.1-rc1"))  # with PR
  min 866.673 ns, mean 959.908 ns (16 allocations, 728 bytes)
v"2.0.1-rc1"

julia> @btime tryparse(VersionNumber, $("2.0.1-rc1"))  # with PR + LazyString for all error messages
  min 862.069 ns, mean 943.470 ns (16 allocations, 728 bytes)
v"2.0.1-rc1"

@topolarity
Copy link
Member Author

topolarity commented May 7, 2024

The major improvement is in compile-time for varied VersionStrings:

julia> version_strings = String[]

julia> for i = 1:1000
           version = "2.0.1"
           prerelease_segments = rand(0:5)
           if prerelease_segments > 0
               segments = ntuple(_->rand(Bool) ? string("rc") : string(rand(1:10)), prerelease_segments)
               version *= "-" * join(segments, ".")
           end
           # version = "2.0.1" or "2.0.1-rc.2" or etc.
           push!(version_strings, version)
       end

julia> @time for s in version_strings
            Base.tryparse(VersionNumber, s)
       end
  0.001412 seconds (18.53 k allocations: 837.891 KiB)

versus on 1.10:

julia> @time for s in version_strings
            Base.tryparse(VersionNumber, s)
       end
  0.745204 seconds (760.38 k allocations: 53.482 MiB, 1.33% gc time, 98.82% compilation time)

a 500x improvement

@topolarity topolarity merged commit 5f7bfc0 into master May 7, 2024
7 checks passed
@topolarity topolarity deleted the ct/despecialize-VersionNumber-constructors branch May 7, 2024 21:17
xlxs4 pushed a commit to xlxs4/julia that referenced this pull request May 9, 2024
…54386)

There's no need to specialize this code for every set of Tuple types
that it might see, especially when the primary callers don't know their
Tuple type any more precisely than this anyway.

Eliminates a dynamic dispatch from `tryparse(VersionNumber, "...")`

Co-authored-by: Gabriel Baraldi <baraldigabriel@gmail.com>
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Jul 12, 2024
…54386)

There's no need to specialize this code for every set of Tuple types
that it might see, especially when the primary callers don't know their
Tuple type any more precisely than this anyway.

Eliminates a dynamic dispatch from `tryparse(VersionNumber, "...")`

Co-authored-by: Gabriel Baraldi <baraldigabriel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants