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

Yet more inference/invalidation fixes #1869

Merged
merged 8 commits into from
Jul 1, 2020
Merged

Yet more inference/invalidation fixes #1869

merged 8 commits into from
Jul 1, 2020

Conversation

timholy
Copy link
Member

@timholy timholy commented Jun 16, 2020

This is the Pkg part of what I found in my investigations of invalidations due to extending ==, c.f. JuliaLang/julia#36282. This also requires JuliaLang/julia#36280, so if that's not on nightly yet this wiil fail tests. (It also requires JuliaLang/julia#36188, but I imagine that's on nightly by now.)

This will be easiest to review ignoring whitespace changes, due to some big let blocks from circumventing more cases of the infamous 15276.

src/Types.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Member Author

timholy commented Jun 16, 2020

Ah, does Pkg get deployed to older Julia versions? Or just, sometimes developed on older Julia versions? Should I add version checks?

@KristofferC
Copy link
Member

Should I add version checks?

Depends on how much extra work there is. We might need to lose 1.4 and 1.5 compat soon anyway when we start using JuliaLang/julia#36018.

@timholy
Copy link
Member Author

timholy commented Jun 16, 2020

It wouldn't be much work, but your TOML-parser sounds like critical infrastructure to me. So perhaps this can just hang out until you're ready to commit to that and Julia 1.6.

@timholy timholy force-pushed the teh/inval3 branch 2 times, most recently from e2aee5c to cd42436 Compare June 23, 2020 16:07
@@ -1749,8 +1749,8 @@ function status(ctx::Context, pkgs::Vector{PackageSpec}=PackageSpec[];
old_ctx = Context(;env=env_diff)
end
# display
filter_uuids = [pkg.uuid for pkg in pkgs if pkg.uuid !== nothing]
filter_names = [pkg.name for pkg in pkgs if pkg.name !== nothing]
filter_uuids = [pkg.uuid::UUID for pkg in pkgs if pkg.uuid !== nothing]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filter_uuids = [pkg.uuid::UUID for pkg in pkgs if pkg.uuid !== nothing]
filter_uuids = UUID[pkg.uuid for pkg in pkgs if pkg.uuid !== nothing]

maybe? Same below.

Copy link
Member Author

Choose a reason for hiding this comment

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

The version here avoids one runtime dispatch and one invalidation-worthy call to convert (from inside the setindex! call):

julia> f(x) = Int[x]
f (generic function with 1 method)

julia> g(x) = [x::Int]
g (generic function with 1 method)

julia> code_typed(f, (Integer,))
1-element Array{Any,1}:
 CodeInfo(
1%1 = $(Expr(:foreigncall, :(:jl_alloc_array_1d), Array{Int64,1}, svec(Any, Int64), 0, :(:ccall), Array{Int64,1}, 1, 1))::Array{Int64,1}
│        Base.setindex!(%1, x, 1)::Any
└──      return %1
) => Array{Int64,1}

julia> code_typed(g, (Integer,))
1-element Array{Any,1}:
 CodeInfo(
1 ─      Core.typeassert(x, Main.Int)::Int64%2 = π (x, Int64)
│   %3 = $(Expr(:foreigncall, :(:jl_alloc_array_1d), Array{Int64,1}, svec(Any, Int64), 0, :(:ccall), Array{Int64,1}, 1, 1))::Array{Int64,1}
│        Base.arraysize(%3, 1)::Int64
└──      goto #3 if not true
2 ─      Base.arrayset(false, %3, %2, 1)::Array{Int64,1}
3 ┄      goto #4
4return %3
) => Array{Int64,1}

Copy link
Member

Choose a reason for hiding this comment

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

Alright, thanks for exaplining.

@KristofferC
Copy link
Member

I think we should drop 1.4 and merge this. We should probably then add a matrix entry to the Travis runner so that we also run with JULIA_PKG_SERVER="" to make sure that it doesn't regress. Also, we can probably get rid of AV in favor of Windows on Travis.

@KristofferC
Copy link
Member

I'll push to this branch for the stuff I mentioned in the last comment

@timholy
Copy link
Member Author

timholy commented Jun 30, 2020

Are you fixing the conflict or shall I? I don't want to cause more trouble by trying to be helpful.

@timholy
Copy link
Member Author

timholy commented Jun 30, 2020

Looks like this should pass. I therefore took the liberty of reorganizing the commits into more logical form. Feel free to merge or squash, whatever you prefer.

Workaround for #15276
JuliaLang/julia#36280 introduced the ability to
pre-allocate the container used to track values of `f.(itr)` in
`unique(f, itr)`. Particularly for containers with `Union` elements,
this circumvents significant inference problems.

Related: JuliaLang/julia#36454
This reduces the vulnerability to invalidations.
Avoids a major source of invalidations
@timholy
Copy link
Member Author

timholy commented Jun 30, 2020

Looks good to go. Might want to increase the amount of time on windows, since one of the two jobs timed out and the other was fairly close to the limit.

@KristofferC
Copy link
Member

I'll put back AV then, it seems faster there.

@timholy
Copy link
Member Author

timholy commented Jul 1, 2020

BTW, might be worth mentioning that after I reconfigured the commits, I'm pretty sure the only two that need nightly are the unique and maximum commits. Let me know if you want me to do anything with that observation.

@KristofferC KristofferC merged commit 20f9b9e into master Jul 1, 2020
@KristofferC KristofferC deleted the teh/inval3 branch July 1, 2020 16:15
@timholy timholy added the latency label Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants