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

1.8.5 -> 1.9 regression in number of allocations from getproperty #49697

Closed
BioTurboNick opened this issue May 9, 2023 · 17 comments
Closed

1.8.5 -> 1.9 regression in number of allocations from getproperty #49697

BioTurboNick opened this issue May 9, 2023 · 17 comments
Milestone

Comments

@BioTurboNick
Copy link
Contributor

BioTurboNick commented May 9, 2023

The execution time is similar for this small example, but there are 10x more allocations on 1.9-rc3.

using BenchmarkTools
using Plots
const xx = randn(20, 3000)
@btime plot(xx; color = :blue, label = nothing);

# 1.8.5
# 1.214 s (392640 allocations: 31.88 MiB)

# 1.9-rc3
# 1.166 s (3964617 allocations: 86.43 MiB)

From JuliaPlots/Plots.jl#4742

@KristofferC
Copy link
Sponsor Member

This needs more of a reduction for this to be actionable over just a @btime plot.

@BioTurboNick
Copy link
Contributor Author

Working on it

@BioTurboNick
Copy link
Contributor Author

The allocations are almost entirely originating from getproperty with UnknownType, according to @profview_allocs.
Screenshot 2023-05-09 105936

Specifically, this method: https://github.com/JuliaLang/julia/blob/master/base/Base.jl#L37

@BioTurboNick BioTurboNick changed the title 1.8.5 -> 1.9-rc3 regression in number of allocations 1.8.5 -> 1.9-rc3 regression in number of allocations from getproperty May 9, 2023
@BioTurboNick
Copy link
Contributor Author

Possibly related: #49241

@KristofferC
Copy link
Sponsor Member

Possibly related: #49241

Could check if the offending commit bisected in that PR (#47371) is the cause by looking at the results one commit before it. Could also try to bisect this but I suspect it will be a bit difficult since it might not be one commit that is to blame for the whole allocation increase.

@BioTurboNick
Copy link
Contributor Author

I boiled it down to a simple demonstration:

const s = Set()
@time s.dict # 0.000003 seconds (1 allocation: 16 bytes)

Bisect pinpointed #47371 as you thought.

@KristofferC
Copy link
Sponsor Member

KristofferC commented May 10, 2023

Putting that in a function makes it not appear anymore

julia> s = Set();

julia> f(s) = s.dict;

julia> @time f(s);
  0.000004 seconds

So how does this manifest itself within Plots? Is it because s is not inferred proerly where the getproperty is called or something like that?

@BioTurboNick
Copy link
Contributor Author

Current master (e204e20) still has it

@BioTurboNick
Copy link
Contributor Author

BioTurboNick commented May 10, 2023

Via

iterate(s::Set, i...) = iterate(KeySet(s.dict), i...)

i = skip_deleted(v.dict, i)

and

vals = T <: KeySet ? v.dict.keys : v.dict.vals

@BioTurboNick
Copy link
Contributor Author

@BioTurboNick
Copy link
Contributor Author

The constant is a Set{Symbol}

@BioTurboNick
Copy link
Contributor Author

Yeah, the cases responsible are all iterating over global constant sets of symbols, or over keys() of a global constant Dict{Symbol, Any},

@3f6a
Copy link

3f6a commented May 10, 2023

Milestone 1.10 means no chance this gets fixed on a 1.9.x ?

@KristofferC
Copy link
Sponsor Member

No that's not what it means. It means that it is a release blocker for 1.10. That's all.

@3f6a
Copy link

3f6a commented May 10, 2023

Ok thanks, sorry for the noise.

@KristofferC
Copy link
Sponsor Member

@BioTurboNick, could you check how the situation is on the #50507 branch?

@oscardssmith oscardssmith changed the title 1.8.5 -> 1.9-rc3 regression in number of allocations from getproperty 1.8.5 -> 1.9 regression in number of allocations from getproperty Jul 12, 2023
@BioTurboNick
Copy link
Contributor Author

@KristofferC Yes it appears to be fixed!

The OP example on the backports-release-1.9 branch:

(393150 allocations: 32.11 MiB)

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

No branches or pull requests

3 participants