Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Remove single argument Pair constructor which invalidates #1581

Closed

Conversation

ChrisRackauckas
Copy link
Contributor

All of the Base Pair constructors are 2 arguments, so the single argument one seems to cause invalidation which then infect the precompilation of all downstream packages.

Before:

using SnoopCompile
using DiffEqBase, RecursiveFactorization
invalidations = @snoopr using OrdinaryDiffEq
length(invalidations) # 61
trees = invalidation_trees(invalidations)

1-element Vector{SnoopCompile.MethodInvalidations}:
 inserting Pair(e::LightGraphs.SimpleGraphs.AbstractSimpleEdge) in LightGraphs.SimpleGraphs at C:\Users\accou\.julia\packages\LightGraphs\IgJif\src\SimpleGraphs\simpleedge.jl:26 invalidated:
   mt_backedges: 1: signature Tuple{Type{Pair}, Vararg{Any}} triggered MethodInstance for (::Base.var"#6#7"{Pair})(::Any) (23 children)
   1 mt_cache

After:

length(invalidations) # 14
trees = invalidation_trees(invalidations) # zero

This only caused a 0.5 second compile time drop to OrdinaryDiffEq.jl, but it is nice and this should be fairly pervasive since Pair is used... well... everywhere. Also, I checked and this Pair constructor isn't even documented in the public API and does not seem to be used in the private API (the error messages even avoid using it), so it seems like just removing it is the best idea.

All of the Base Pair constructors are 2 arguments, so the single argument one seems to cause invalidation which then infect the precompilation of all downstream packages. 

Before:

```julia
using SnoopCompile
using DiffEqBase, RecursiveFactorization
invalidations = @Snoopr using OrdinaryDiffEq
length(invalidations) # 61
trees = invalidation_trees(invalidations)

1-element Vector{SnoopCompile.MethodInvalidations}:
 inserting Pair(e::LightGraphs.SimpleGraphs.AbstractSimpleEdge) in LightGraphs.SimpleGraphs at C:\Users\accou\.julia\packages\LightGraphs\IgJif\src\SimpleGraphs\simpleedge.jl:26 invalidated:
   mt_backedges: 1: signature Tuple{Type{Pair}, Vararg{Any}} triggered MethodInstance for (::Base.var"sbromberger#6#7"{Pair})(::Any) (23 children)
   1 mt_cache
```

After:

```julia
length(invalidations) # 14
trees = invalidation_trees(invalidations) # zero
```

This only caused a 0.5 second compile time drop to OrdinaryDiffEq.jl, but it is nice and this should be fairly pervasive since `Pair` is used... well... everywhere. Also, I checked and this `Pair` constructor isn't even documented in the public API and does not seem to be used in the private API (the error messages even avoid using it), so it seems like just removing it is the best idea.
@ChrisRackauckas
Copy link
Contributor Author

Found in SciML/DiffEqBase.jl#698

@sbromberger
Copy link
Owner

I guess I don't quite understand. It's not type piracy since I'm extending Pair for use with my own defined datatype. Multiple dispatch should take care of this. Why should this cause a problem?

@ChrisRackauckas
Copy link
Contributor Author

It's not piracy but method invalidations. The compiler seems to specialize on the signature Tuple{Type{Pair}, Vararg{Any}} via (::Base.var"#6#7"{Pair})(::Any) (the Pair constructor itself?) which then gets invalidated by having a further specialization on any code that constructs a Pair. @vtjnash thinks this might be able to be fixed at the compiler level, making it never specialize Tuple{Type{Pair}, Vararg{Any}}, but the core result is that the existence of these dispatches causes downstream code with Pairs/Dicts/etc. to have to recompile any compilation caches, and this thus seems to be a big cause of invalidating precompilation.

@sbromberger
Copy link
Owner

So I'm not opposed to removing the constructor - as you said, it's not part of the API - but this still sounds like it's a fundamental issue with the language that we're not really free to take advantage of multiple dispatch to extend constructors. This is one of the big selling points of Julia, after all - and now we're seeing that a single implementation in one package is causing widespread problems across a wide swath of the ecosystem.

I'm probably missing something.

@timholy
Copy link

timholy commented Aug 11, 2021

I'll fix this in Base+stdlib instead. This seems like a legit use, so I don't think this package needs to change. Instead, we should fix the callers. So far it looks like all the problematic cases are coming from Pkg, and mostly due to the infamous JuliaLang/julia#15276.

Thanks for identifying a problem @ChrisRackauckas! But I think this specific PR can be closed.

@ChrisRackauckas
Copy link
Contributor Author

Alrighty, cool! Yeah, I don't mean to worry anyone. SciML/DiffEqBase.jl#698 is kind of just a compile time rampage and I'm trying to document everything I run into.

@ChrisRackauckas ChrisRackauckas deleted the invalidations branch August 11, 2021 14:50
@timholy
Copy link

timholy commented Aug 11, 2021

I'll CC you on the Pkg PR just so you see how this is done. In general the strategy is to use ascend to find the caller where type inference is lost, and then fix the inference problem. Methods that are well-inferred can't be invalidated except by type-piracy (and this is definitely not such a case). Fixing things this way is a harder road, but the good news is that it makes the invalidated code faster/smaller/better without forcing any changes in the package ecosystem. So basically you can use invalidations as a way of discovering what needs to be made more bullet-proof.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants