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

Improve type stability for tryparse VersionNumber #40557

Merged
merged 2 commits into from Apr 22, 2021
Merged

Improve type stability for tryparse VersionNumber #40557

merged 2 commits into from Apr 22, 2021

Conversation

rikhuijzer
Copy link
Contributor

I noticed that LaTeXStrings is causing 297 method invalidations:

inserting firstindex(s::LaTeXString) in LaTeXStrings at /home/rik/git/LaTeXStrings.jl/src/LaTeXStrings.jl:108 invalidated:
   backedges: 1: superseding firstindex(s::AbstractString) in Base at /nix/store/31534zna4n6p2s0jwadn8jl6lkvxqjch-julia_16/share/julia/base/strings/basic.jl:180 with MethodInstance for firstindex(::AbstractString) (297 children)

Specifically, the invalidation tree starts with

 firstindex(::AbstractString)
   match(::Regex, ::AbstractString)
     tryparse(::Type{VersionNumber}, ::AbstractString)

If I understand invalidations correctly, this invalidation will happen for any package which defines a new firstindex(::T) where T isa AbstractString. Although they can fix it by also defining match(r::Regex, T); it is probably a good idea to avoid these invalidations altogether.

base/version.jl Outdated Show resolved Hide resolved
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
@rikhuijzer
Copy link
Contributor Author

I think that the error on linx32 is unrelated, because the same occurs at #40523.

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

Did you confirm with SnoopCompile that this fixes the invalidations? Otherwise LGTM

@rikhuijzer
Copy link
Contributor Author

Did you confirm with SnoopCompile that this fixes the invalidations?

No, sorry. I did not do that. That would require building Julia from source and then running SnoopCompile on the package again, am I right?

@jebej
Copy link
Contributor

jebej commented Apr 22, 2021

Wouldn't all the other methods that call firstindex still get invalidated here? IIUC this change would only protect tryparse.

@jebej
Copy link
Contributor

jebej commented Apr 22, 2021

I think I misunderstood: the methods invalidated are methods that call tryparse.

@simeonschaub
Copy link
Member

No, sorry. I did not do that. That would require building Julia from source and then running SnoopCompile on the package again, am I right?

You could actually also click on Details of the tester for your specific OS and then look up the download_url under the Build Properties tab, which will contain the built binaries. Just as a tip. But otherwise, yes, that's how you would do it.

@jebej
Copy link
Contributor

jebej commented Apr 22, 2021

On the CI build:

inserting firstindex(s::LaTeXString) in LaTeXStrings at C:\Users\Jeremy\.julia\packages\LaTeXStrings\anRaX\src\LaTeXStrings.jl:108 invalidated:
   backedges: 1: superseding firstindex(s::AbstractString) in Base at strings/basic.jl:180 with MethodInstance for firstindex(::AbstractString) (1 children)

@KristofferC KristofferC merged commit 1fbb536 into JuliaLang:master Apr 22, 2021
@rikhuijzer
Copy link
Contributor Author

You could actually also click on Details of the tester for your specific OS and then look up the download_url under the Build Properties tab, which will contain the built binaries. Just as a tip. But otherwise, yes, that's how you would do it.

Thank you for the advise. I will do that before I submit any new PRs on invalidations. 👍

On the CI build:

inserting firstindex(s::LaTeXString) in LaTeXStrings at C:\Users\Jeremy\.julia\packages\LaTeXStrings\anRaX\src\LaTeXStrings.jl:108 invalidated:
   backedges: 1: superseding firstindex(s::AbstractString) in Base at strings/basic.jl:180 with MethodInstance for firstindex(::AbstractString) (1 children)

Thank you for checking!

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Co-authored-by: Simeon Schaub <simeondavidschaub99@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.

None yet

4 participants