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

Excise Random from the system image #51432

Closed
wants to merge 3 commits into from
Closed

Excise Random from the system image #51432

wants to merge 3 commits into from

Conversation

vchuravy
Copy link
Member

I am not really a fan of this solution,
but it does work. Preferably this is something one could
express to the method-table so that instead of doing this at init time
in Random, we do it on at top-level and package loading does the deletion
and addition of methods at the same time.

For Random the current solution might be fine, but for LinearAlgebra we do
need a better mechanism.

@vchuravy vchuravy added excision Removal of code from Base or the repository stdlib Julia's standard library labels Sep 23, 2023
@rfourquet
Copy link
Member

What a beautiful hack!

For Random the current solution might be fine, but for LinearAlgebra we do need a better mechanism.

Why wouldn't that work well for LinearAlgebra?

base/Base.jl Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member Author

Why wouldn't that work well for LinearAlgebra?

We currently need to run delete_method as part of the init call, and can't use it during precompilation. So this may lead to unnecessary method invalidation. For rand the impact might not be too bad, but I am worried about *.

Ideally we could express method deletion to precompilation and perform it along side method insertion and not invalidate the method table after loading the package once again.

Quoting @vtjnash

There needs to be a sequenced-before event for the deletion

But I don't think I have the bandwidth to implement that.

@Keno
Copy link
Member

Keno commented Sep 26, 2023

I happened to discuss this today with @gbaraldi @staticfloat and a few others. The big problems with this approach is that this isn't really compatible with pre-compilation. We discussed whether it would be possible to have a special method marker that causes the runtime to load a pkgimage when something looks at the method (either inference or the runtime). If we pre-declare all methods that the pkgimage is adding to base and use this mechanism for them, I don't think we actually need to bump the world age, so this should be compatible with pre-compilation. Of course, we'd also need a mechanism to prevent the pkgimage from actually adding any methods to base that weren't predeclared, but that seems easy to do.

@vchuravy
Copy link
Member Author

Agreed, when I discussed this with @vtjnash he seemed to think that it would suffice to have a method deletion marker, such that the deletion happens before we insert the new methods in the MT.

In was also thinking fo a special kind of method that knows to load a different library, but that seems to be a bigger ask. (E.g. what do we do when @stdlib is not on the load path)

I would definitely appreciate some help here with the runtime changes, in particular we need to solve this for Linear algebra where the numbers of methods are.many more and the cost of invalidation is much higher.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 27, 2023

it would be possible to have a special method marker that causes the runtime to load a pkgimage when something looks at the method

I don't really see how that it is different from having the package actually just loaded. It is not like the methods somehow actively do things when you aren't using them, they are just special markers with data that define what things happen when someone actively intentionally looks at them.

That is a bit different from the LinAlg situation perhaps, where we almost want an LBT-like approach (lazy blas trampoline) so that you can start with just *, and then later define what it means for matmul at runtime, opaque to the compiler and swappable live.

@vchuravy
Copy link
Member Author

vchuravy commented Oct 8, 2023

help?> randn
search: randn rand tand range nand rounding

  No documentation found for public symbol.

  Base.randn is a Function.

  # 1 method for generic function "randn" from Base:
   [1] randn(args...)
       @ Base.Stubs.Random stubs.jl:20

Need to fix that, probably by docs noticing and calling Base.Stubs.Random.delay_initialize

base/stubs.jl Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member Author

@timholy Test failures here are due to test/prcompile.jl using rand() and the assumptions not holding anymore. Should I replace rand() with some other effectful function?

@KristofferC https://github.com/JuliaLang/Pkg.jl/blob/b02fb95979c71dc5834aad739ad61622cccf4a16/test/api.jl#L274 assumes that Random is in the system image? I assume deleting that test should be fine?

@vchuravy
Copy link
Member Author

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@KristofferC
Copy link
Sponsor Member

I assume deleting that test should be fine?

Yes, that seems fine.

@vchuravy
Copy link
Member Author

I am inclined towards merging this next week to start shaking out issues with this on nightlies.

@vtjnash or @JeffBezanson does either of you have time for a review?

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Nov 22, 2023

Given that this has a performance cost for rand or or randn until you do import Random, and then though not after, is it maybe not worth it to do this, this way i.e. fully excising, as opposed to my alternative below? [Random doesn't need to be added to an environment, e.g. for benchmarking scripts(?)] Unlike excising LinearAlgebra and OpenBLAS which is really important.

What's the cost of having rand (but just for Xoshiro) in the sysimage? randn does only have 26% overhead over rand, so probably should be kept too (I don't know about the code overhead of it, likely larger but still small).

Given that Xoshiro, the default rng is really simple and small (at least for uniform), both its state and the code, is it possible to excise Random partially from the sysimage? I.e. the legacy rng, and all that's not usable before the import, i.e. what's not strictly in Base, but feels like it. I think I'm just proposing moving rand and rand to Base (from Random), and keep leaving the rest e.g. randstring out.

I think it would mean you could fully quality as Base.rand as you already can (or not qualify). You can also qualify as Random.rand and it should also be kept, seems simple and not slower.

vchuravy and others added 3 commits January 22, 2024 10:43
…51641)

The problem with the `delete_method` in `__init__` approach is that we
invalidate the method-table,
after we have performed all of the caching work. A package dependent on
`Random`, will still see
the stub method in Base and thus when we delete the stub, we may
invalidate useful work.

Instead we delete the methods when Random is being loaded, thus a
dependent package only ever sees
the method table with all the methods in Random, and non of the stubs
methods.

The only invalidation that thus may happen are calls to `rand` and
`randn` without first doing an `import Random`.
@vchuravy vchuravy closed this Mar 11, 2024
@giordano giordano deleted the vc/excise_random branch June 19, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
excision Removal of code from Base or the repository stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants