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

use a fixed world for code loading #49525

Merged
merged 3 commits into from Jun 9, 2023
Merged

use a fixed world for code loading #49525

merged 3 commits into from Jun 9, 2023

Conversation

KristofferC
Copy link
Sponsor Member

Code loading Base.require is one function that keeps getting run over and over as packages gets loaded. It is therefore important that this function does not get invalidated because having to recompile it causes significant latency. A lot of work has been done trying to shield it from invalidations but it seems to be a never ending battle. During the loading of OmniPackage.jl for examples, I have seen code loading gotten recompiled 4 times during the loading of that package.

A possible solution is to run the code loading code in a fixed world age thereby making it immune to invalidations (since it keeps running the old code). This also means that the code cannot be straight forwardly Revised, but maybe that is a price worth paying.

cc @c42f, @timholy, @JeffBezanson, @vtjnash

@KristofferC KristofferC added the domain:packages Package management and loading label Apr 26, 2023
Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

This seems like a good tradeoff to me.

If people want to circumvent this when they're developing the loading code, can't they just do

Base._require_world_age[] = typemax(UInt)

?

base/loading.jl Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member

can this also be done for repl tab completions?

@c42f
Copy link
Member

c42f commented Apr 27, 2023

This can and probably should be done for any Julia "session infrastructure" or compiler-related stuff which is

  • Implemented on the Julia side
  • Does not need to be extended by users
  • Suffers from invalidations due to package loading

REPL keybindings are an interesting case - we can't choose a fixed world for all key bindings because packages add to the keybinding mappings. Rather, packages which extend the keybindings need to choose an appropriate world age when the keybinding is installed. For example I did this for OhMyREPL in KristofferC/OhMyREPL.jl#321

If the REPL had an API for adding keybindings, it would make sense to capture the world age when the keybinding was added and to replace the current use of invokelateset() within the REPL with invoke_in_world.

However that's a separate PR.

@KristofferC
Copy link
Sponsor Member Author

After #49604, I am not sure I have a concrete case in the wild where this is needed.

@c42f
Copy link
Member

c42f commented May 3, 2023

I still think it's a good idea to shield this (and other) parts of the Julia core implementation from user invalidations. (With an opt-out for Base developers, which we do have here.)

We already have this for Core.Compiler, and basically have had for a long time before invoke_in_world existed (implemented at the C level). Why not also have it for other critical parts of the runtime?

@KristofferC
Copy link
Sponsor Member Author

Okay, I am seeing require getting invalidated again now so now I want to do this again hah.

@c42f
Copy link
Member

c42f commented May 28, 2023

I found a neat way to express the fixed world age in

JuliaLang/JuliaSyntax.jl#290

So an option would be to steal the fix_world_age function from there, if it helps:

https://github.com/JuliaLang/JuliaSyntax.jl/blob/68d3fbd300fd3d03e69bfc1e99bb7eb7dcd47030/src/hooks.jl#L107

@KristofferC
Copy link
Sponsor Member Author

Hm, for this specific case, I don't think it brings much clarity over just doing it manually.

@KristofferC
Copy link
Sponsor Member Author

@nanosoldier runtests(vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@KristofferC
Copy link
Sponsor Member Author

Let's try this then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants