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

Fix regression of Pkg and more #55721

Closed
wants to merge 1 commit into from

Conversation

PallHaraldsson
Copy link
Contributor

@PallHaraldsson PallHaraldsson commented Sep 9, 2024

Please add to 1.11 backport, because (Markdown and) Pkg are used by code, such as PythonCall (indirectly), not just interactively.

This is emitted for Pkg and Markdown:
precompile(Tuple{typeof(Base.setindex!), Base.EnvDict, Bool, String})

This one too for Pkg:
precompile(Tuple{typeof(Base.first), Array{Any, 1}})

I do not add to Pkg section, it and REPL one seem should not exist... from when they where in the sysimage? Those precompiles should still likely still be in, since useful for other code, might be moved into misc too.

This may not be strictly needed, I've (only?) seen this when exiting the REPL:
precompile(Tuple{typeof(Base.print), Base.TTY, String})

I didn't time any of these yet, a difference when in the sysimage, or how much larger it gets, e.g. for that last one.

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Sep 9, 2024

[This is separate from #55715 (see also #55706), though both related (was just objected to mentioning Pkg there, so I do not solve this all in one PR), could be merged as one, at least it builds on it (for Markdown), I think neither here or there alone is best for Markdown.]

@KristofferC
Copy link
Sponsor Member

You say this fixes a regression? What is the regression and what are the before and after numbers with this PR?

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Sep 9, 2024

If I do:

$ julia +1.11

(@v1.11) pkg>

exit the package mode, then:

julia> @time using CondaPkg
  0.216666 seconds (118.56 k allocations: 7.610 MiB, 14.91% compilation time)

but if I do not enter Pkg mode first then:

julia> @time using CondaPkg
  1.156448 seconds (667.36 k allocations: 42.288 MiB, 13.75% gc time, 2.74% compilation time)

and actually way worse like this (and I'm working on that, at least trying to understand and fix, I want that 0.2 sec...):

$ time julia +1.11 -e "using CondaPkg"

real	0m3,556s
user	0m4,280s
sys	0m0,170s

$ time julia +1.11 -e "@time using CondaPkg"
  3.487787 seconds (1.89 M allocations: 105.751 MiB, 4.69% gc time, 74.62% compilation time)

real	0m4,019s
user	0m4,674s
sys	0m0,240s

It would be nice to be able to use `@time_imports` to debug that difference, but it only works in the REPL (why? could that be fixed?), so it's harder to know why so much longer, to pin-point the reason. CondaPkg, isn't too important when using alone, but it's a dependency of PythonCall, so I'm really working on its latency... and it's 8 sec, 4.5 sec in the REPL.

That's the regression I'm working on. [But while I enter pkg mode immediately, seemingly, I think it's because of async, so a bit deceiving, why this regression hasn't been seen, or is known but papered over. I realize you don't want Pkg in the sysimage, and I agree, but it has its own pkgimage?! Also the precompile is only for Base, not Pkg, specifically, though I think it benefits from it.]

I haven't confirmed that this fixes this regression, but CondaPkg has Pkg as a dependency, this might, but I haven't tested locally yet. I think it should since, locally, without this:

$ time julia +1.11 --trace-compile=stderr -e "using Pkg"
precompile(Tuple{typeof(Base.setindex!), Base.EnvDict, Bool, String})
precompile(Tuple{typeof(Base.first), Array{Any, 1}})

real	0m0,883s
user	0m1,629s
sys	0m0,150s

@KristofferC
Copy link
Sponsor Member

I haven't confirmed that this fixes this regression

Okay but the title of the PR says "Fix regression" so how do you know this is true if you haven't confirmed it?

These precompile signatures execute very fast so I am surprised they would do much at all?

julia> @time precompile(Tuple{typeof(Base.setindex!), Base.EnvDict, Bool, String})
  0.002380 seconds (938 allocations: 46.578 KiB, 99.33% compilation time)
true

julia> @time precompile(Tuple{typeof(Base.first), Array{Any, 1}})
  0.003230 seconds (87 allocations: 3.812 KiB, 99.64% compilation time)
true

julia> @time precompile(Tuple{typeof(Base.print), Base.TTY, String})
  0.002157 seconds (72 allocations: 2.859 KiB, 99.72% compilation time)
true

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Sep 9, 2024

Okay but the title of the PR says "Fix regression" so how do you know this is true if you haven't confirmed it?

Ok, sorry about that, maybe turn this into an issue? I assumed it fixes, since I only saw those with --trace-compile=stderr, and I'm stumped on what else is needed to fix this.

Do you know what it might be? Feel free to answer and/or close this PR if it's useless, might not be possible to migrate it to an issue.

@KristofferC
Copy link
Sponsor Member

Do you know what it might be?

Pkg is not in the sysimage anymore so it costs a bit to load it.

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.

2 participants