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

Fix non-concrete type near TRCache #118

Merged
merged 3 commits into from
Feb 16, 2022
Merged

Fix non-concrete type near TRCache #118

merged 3 commits into from
Feb 16, 2022

Conversation

rikhuijzer
Copy link
Contributor

@rikhuijzer rikhuijzer commented Feb 15, 2022

Very minor change. This fixes non-concrete types reported by

@code_warntype TapedTask(f, args...)

and

@code_warntype TapedFunction(f, args...; cache=true, init=true)

The compiler wasn't able to determine the type of TRCache[cache_key] which makes sense.

Also, I've tried to narrow down the types of TRCache which makes things slightly easier for the readers and the compiler.

Same for TapedTaskException.backtrace.

@devmotion
Copy link
Member

The types, e.g., of the cache, are still non-concrete though. Somewhere I read recently that there are cases where it is better to make/leave non-concrete types as Any instead of using another more restrictive non-concrete type... I am not sure if it applies to any of the changes in this PR though.

It sounds like this PR is supposed to fix some type inference issue though? Can we add tests for it?

@rikhuijzer
Copy link
Contributor Author

rikhuijzer commented Feb 15, 2022

It sounds like this PR is supposed to fix some type inference issue though? Can we add tests for it?

I think yes. Test via JETTest or is there some other method? EDIT: Found it. It's Test.@inferred. Added in 692d574.

I’ll respond to your other comments tomorrow

Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

thanks @rikhuijzer - it looks good to me!

@yebai
Copy link
Member

yebai commented Feb 15, 2022

It'll be great to gradually fix the type instability issues in TapedFunction. It also helps to keep these PRs small (this PR is an excellent example) so they can be reviewed and merged quickly.

@rikhuijzer
Copy link
Contributor Author

The types, e.g., of the cache, are still non-concrete though. Somewhere I read recently that there are cases where it is better to make/leave non-concrete types as Any instead of using another more restrictive non-concrete type... I am not sure if it applies to any of the changes in this PR though.

Maybe, you've read SciML/ModelingToolkit.jl#1215 (comment)? I've tried creating the TRCache via

const TRCache = LRU{Tuple, TapedFunction{Any}}(maxsize=10)

but that gave a "could not convert" errror.

@devmotion
Copy link
Member

but that gave a "could not convert" errror.

That's expected since TapedFunction{F} for a concrete F (and even TapedFunction{F} where F) is not a subtype of TapedFunction{Any}, so it would have to be converted: https://docs.julialang.org/en/v1/manual/types/#man-parametric-composite-types

Maybe, you've read SciML/ModelingToolkit.jl#1215 (comment)?

Hmm, I don't think so, but it was a similar comment somewhere. In any case, with respect to the issue mentioned in the PR the change in this PR is no improvement, since the cache already satisfied isconcretetype:

julia> using LRUCache

julia> cache1 = LRU{Any,Any}(maxsize=10);

julia> typeof(cache1)
LRU{Any, Any}

julia> isconcretetype(typeof(cache1))
true

julia> struct T{F}
           f::F
       end

julia> cache2 = LRU{Tuple,T}(maxsize=10);

julia> isconcretetype(typeof(cache2))
true

julia> typeof(cache2)
LRU{Tuple, T}

Of course, the PR changes the (non-)inferred return type of cache[key] (in both cases it can't be inferred exactly):

julia> cache1[(:a, 1)] = T(2.0)
T{Float64}(2.0)

julia> @code_warntype cache1[(:a, 1)]
MethodInstance for getindex(::LRU{Any, Any}, ::Tuple{Symbol, Int64})
  from getindex(lru::LRU, key) in LRUCache at /home/david/.julia/packages/LRUCache/44dJX/src/LRUCache.jl:125
Arguments
  #self#::Core.Const(getindex)
  lru::LRU{Any, Any}
  key::Tuple{Symbol, Int64}
Locals
  #12::LRUCache.var"#12#13"{LRU{Any, Any}, Tuple{Symbol, Int64}}
Body::Any
1%1 = LRUCache.:(var"#12#13")::Core.Const(LRUCache.var"#12#13")
│   %2 = Core.typeof(lru)::Core.Const(LRU{Any, Any})
│   %3 = Core.typeof(key)::Core.Const(Tuple{Symbol, Int64})
│   %4 = Core.apply_type(%1, %2, %3)::Core.Const(LRUCache.var"#12#13"{LRU{Any, Any}, Tuple{Symbol, Int64}})
│        (#12 = %new(%4, lru, key))%6 = #12::LRUCache.var"#12#13"{LRU{Any, Any}, Tuple{Symbol, Int64}}%7 = Base.getproperty(lru, :lock)::Base.Threads.SpinLock%8 = LRUCache.lock(%6, %7)::Any
└──      return %8

julia> cache2[(:a, 1)] = T(2.0)
T{Float64}(2.0)

julia> @code_warntype cache2[(:a, 1)]
MethodInstance for getindex(::LRU{Tuple, T}, ::Tuple{Symbol, Int64})
  from getindex(lru::LRU, key) in LRUCache at /home/david/.julia/packages/LRUCache/44dJX/src/LRUCache.jl:125
Arguments
  #self#::Core.Const(getindex)
  lru::LRU{Tuple, T}
  key::Tuple{Symbol, Int64}
Locals
  #12::LRUCache.var"#12#13"{LRU{Tuple, T}, Tuple{Symbol, Int64}}
Body::T
1%1 = LRUCache.:(var"#12#13")::Core.Const(LRUCache.var"#12#13")
│   %2 = Core.typeof(lru)::Core.Const(LRU{Tuple, T})
│   %3 = Core.typeof(key)::Core.Const(Tuple{Symbol, Int64})
│   %4 = Core.apply_type(%1, %2, %3)::Core.Const(LRUCache.var"#12#13"{LRU{Tuple, T}, Tuple{Symbol, Int64}})
│        (#12 = %new(%4, lru, key))%6 = #12::LRUCache.var"#12#13"{LRU{Tuple, T}, Tuple{Symbol, Int64}}%7 = Base.getproperty(lru, :lock)::Base.Threads.SpinLock%8 = LRUCache.lock(%6, %7)::T
└──      return %8

@rikhuijzer
Copy link
Contributor Author

Hmm, I don't think so, but it was a similar comment somewhere. In any case, with respect to the issue mentioned in the PR the change in this PR is no improvement, since the cache already satisfied isconcretetype:

Agreed, but it is an improvement with regards to readability.

Of course, the PR changes the (non-)inferred return type of cache[key] (in both cases it can't be inferred exactly):

Yes also correct.

Let's merge?

@devmotion
Copy link
Member

Maybe bump the version number in the PR, otherwise looks good to me 🙂

@rikhuijzer
Copy link
Contributor Author

Maybe bump the version number in the PR, otherwise looks good to me 🙂

Oh yes, I forgot that that is TuringLang convention. Done now.

Thank you for the review! I don't have merge rights here, can one of you do it please?

@yebai yebai merged commit 8db6c3a into master Feb 16, 2022
@delete-merged-branch delete-merged-branch bot deleted the rh/TRCache-type branch February 16, 2022 14:48
@yebai
Copy link
Member

yebai commented Feb 16, 2022

Thanks, @rikhuijzer - you should have merge rights now.

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.

None yet

3 participants