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

polymorphic specializations field: either svec or value #49071

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Mar 20, 2023

Attempt an experiment: remove arrays that only have one value. Seems to save about 0.8% (1MB) in the sysimg, since we were rather significantly over-sizing these (about 16 slots initialized) before.

Also a small lookup-failure optimization for the hashing-failed case to avoid duplicated work.

Attempt an experiment: remove arrays that only have one value. Seems to
save about 0.8% (1MB) in the sysimg, since we were rather significantly
over-sizing these (about 16 slots initialized) before.

Also a small lookup-failure optimization for the hashing-failed case to
avoid duplicated work.
Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

Can we make m.specializations::Union{SingleSpec, SimpleVector} and define iterate for SingleSpec to land this as non-breaking change? It would probably better to run pkgeval to assess the impact of this change on the ecosystem.

@vtjnash
Copy link
Member Author

vtjnash commented Mar 21, 2023

I don't think that is worthwhile. I have made significant changes to this field in most releases (and likely will again), so anyone depending on it without going through the API is already in trouble.

@vtjnash vtjnash merged commit 0b877b7 into master Mar 21, 2023
@vtjnash vtjnash deleted the jn/micro-compact-specs branch March 21, 2023 15:46
@timholy
Copy link
Member

timholy commented Mar 21, 2023

What is the API? I seem to be guilty of not using it: https://juliahub.com/ui/Search?type=code&q=.specializations&w=true

The MethodAnalysis package defines what I'd call a real API: mi = methodinstance(f, tt). But I'm not aware of one in Base.

@vtjnash
Copy link
Member Author

vtjnash commented Mar 21, 2023

AFAIK, it is only defined as Core.Compiler.specialize_method (which is also unstable, but slightly less so)

@timholy
Copy link
Member

timholy commented Mar 21, 2023

OK, I was trying to avoid depending on compiler internals but perhaps that's better in this case.

@timholy
Copy link
Member

timholy commented Mar 21, 2023

There's more: MethodAnalysis.jl can traverse the entire system (or specific modules) and collect all the existing MethodInstances. Is there an approved API for this? I riffed off of Base.visit but we don't (or didn't when I last checked) have a method for visiting all specializations of a Method. That's needed for quite a few analyses & test-suites (https://github.com/JuliaDebug/Cthulhu.jl/blob/master/TypedSyntax/test/exhaustive.jl is one recent example). Many of our internal representations are not fully documented, so the only way to know if, e.g., packages like JuliaInterpreter/CodeTracking/TypedSyntax/Revise can handle all the different constructs is to try a large collection of examples.

@vtjnash
Copy link
Member Author

vtjnash commented Mar 22, 2023

MethodInstance is sufficiently internal, that it does not really have a usable API except for that query

@timholy
Copy link
Member

timholy commented Mar 22, 2023

Ok. Assuming this is sticking around, I'll adapt to the new approach.

@vtjnash
Copy link
Member Author

vtjnash commented Mar 22, 2023

If you do want to add an API for that in Base somewhere as well, I will try to maintain it

@timholy
Copy link
Member

timholy commented Mar 22, 2023

Will do. I think it works well as a visit method for m.specializations.

@ulysses4ever
Copy link
Contributor

I'm both using Tim's MethodAnalysis and occasionally index into specializations (including in tests: kinda what this patch had to handle in https://github.com/JuliaLang/julia/pull/49071/files#diff-7e050c69723b3de58379c7ac90882676badb1dcd11cd74866a0d2d7373a93a7fL289), so a good amount of my code will be broken. Oh well, I'm looking forward to an API and to an update to MehtodAnalysis, 'cause currently my CI fails on nightly inside MethodAnalysis.visit(::Module).

@timholy would it make sense to open a ticket on MethodAnalysis so that interested parties could get a notification when support for nightly will be restored?

ulysses4ever added a commit to prl-julia/julia-type-stability that referenced this pull request Mar 22, 2023
It's currently irrepairable due to

JuliaLang/julia#49071
ulysses4ever added a commit to prl-julia/julia-type-stability that referenced this pull request Mar 23, 2023
It's currently irrepairable due to

JuliaLang/julia#49071
timholy added a commit that referenced this pull request Mar 23, 2023
There have been longstanding reasons to want an API for extracting
all extant specializations of a method, but this is even more
true after #49071.
chriselrod pushed a commit to JuliaSIMD/LoopVectorization.jl that referenced this pull request Mar 23, 2023
This updates for JuliaLang/julia#49071. I added this test, so I thought I should fix it.
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Mar 24, 2023
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Mar 24, 2023
* adjust to JuliaLang/julia#49071

* adjust to JuliaLang/julia#49113

  After JuliaLang/julia#49113, `src.slot[names|types|flags]` may be
  resized by the optimization pipeline, so we have to make sure to store
  our own copies of them to keep the pre-optimization state of `CodeInfo`.
@timholy
Copy link
Member

timholy commented Mar 24, 2023

@timholy would it make sense to open a ticket on MethodAnalysis so that interested parties could get a notification when support for nightly will be restored?

Already fixed (v0.4.12)

@ulysses4ever
Copy link
Contributor

@timholy thank you!

timholy added a commit that referenced this pull request Mar 30, 2023
There have been longstanding reasons to want an API for extracting
all extant specializations of a method, but this is even more
true after #49071.

Co-authored-by: Shuhei Kadowaki <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
)

Attempt an experiment: remove arrays that only have one value. Seems to
save about 0.8% (1MB) in the sysimg, since we were rather significantly
over-sizing these (about 16 slots initialized) before.

Also a small lookup-failure optimization for the hashing-failed case to
avoid duplicated work.
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
There have been longstanding reasons to want an API for extracting
all extant specializations of a method, but this is even more
true after JuliaLang#49071.

Co-authored-by: Shuhei Kadowaki <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
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.

4 participants