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

inferece: remove CachedMethodTable #44240

Merged
merged 1 commit into from
Feb 23, 2022
Merged

inferece: remove CachedMethodTable #44240

merged 1 commit into from
Feb 23, 2022

Conversation

aviatesk
Copy link
Member

Previously the method lookup result was created per frame and so the
look cache hasn't been use that much. With this change the cache is
created per inference, and so the cached result will be used when we
already saw the same match in the same inference shot, and it may speed
up the lookup time a bit.

This commit also setups new AbstractInterpreter interface get_method_lookup_cache
which specifies what method lookup cache is used by each AbstractInterpreter.
NativeInterpreter creates a cache per inference, and so it is valid
since lookup is done in the same world age in the same inference shot.
External AbstractInterpreter doesn't opt into this cache by default,
and its behavior won't change in anyway.

@aviatesk aviatesk added the compiler:inference Type inference label Feb 18, 2022
@aviatesk aviatesk requested a review from Keno February 18, 2022 14:25
@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Is this that helpful anymore? For concrete types, we already cache those in a hash table at the next level

@aviatesk
Copy link
Member Author

I didn't confirm noticeable performance improvements when running the inference benchmark locally.

For concrete types, we already cache those in a hash table at the next level

But does this mean cache might be useful when a signature is abstract?

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member Author

Lol, the benchmark shows that this change make it less efficient both in time and space...
@nanosoldier runbenchmarks("inference", vs=":master")

@vtjnash
Copy link
Member

vtjnash commented Feb 18, 2022

yeah, we might want to do this and have only non-concrete types put here, otherwise we may end up doing the lookup twice in the cache-miss case.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member Author

ok, I really couldn't confirm any performance improvement in CachedMethodTable in the current infrastructure.
Now I'd like to propose to eliminate that entirely, and it should save a bit of space:
@nanosoldier runbenchmarks("inference", vs=":master")

@aviatesk aviatesk changed the title inference: use the same method lookup cache across same inference trial inferece: remove CachedMethodTable Feb 21, 2022
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

base/compiler/types.jl Outdated Show resolved Hide resolved
base/compiler/types.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 22, 2022
Since we couldn't confirm any performance benefit from `CachedMethodTable`
in the current infrastructure (see the benchmark results in #44240), now
I'd like to propose to eliminate that entirely and save a bit of space.
@aviatesk aviatesk merged commit 26b0b6e into master Feb 23, 2022
@aviatesk aviatesk deleted the avi/lookupcache branch February 23, 2022 05:08
aviatesk added a commit to aviatesk/GPUCompiler.jl that referenced this pull request Feb 23, 2022
Apologize for the confusion!
In <JuliaLang/julia#44240>, we figured out that
`CachedMethodTable` doesn't give us any performance benefit (both in
time and space), and so we removed that from Julia Base.
This commit updates the (newly updated) GPUCompiler's overloads accordingly.
pchintalapudi pushed a commit to pchintalapudi/julia that referenced this pull request Feb 24, 2022
Since we couldn't confirm any performance benefit from `CachedMethodTable`
in the current infrastructure (see the benchmark results in JuliaLang#44240), now
I'd like to propose to eliminate that entirely and save a bit of space.
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Feb 25, 2022
aviatesk added a commit that referenced this pull request Mar 1, 2022
…InferenceState)` interface

In #44240 we removed the `CachedMethodTable` support as it turned out to
be ineffective under the current compiler infrastructure.
Because of this, there is no strong reason to keep a method table per `InferenceState`.
This commit simply removes the `method_table(::AbstractInterpreter, ::InferenceState)`
interface and should make it clearer which interface should be overloaded to
implement a contextual dispatch.
aviatesk added a commit that referenced this pull request Mar 1, 2022
…InferenceState)` interface

In #44240 we removed the `CachedMethodTable` support as it turned out to
be ineffective under the current compiler infrastructure.
Because of this, there is no strong reason to keep a method table per `InferenceState`.
This commit simply removes the `method_table(::AbstractInterpreter, ::InferenceState)`
interface and should make it clearer which interface should be overloaded to
implement a contextual dispatch.
aviatesk added a commit that referenced this pull request Mar 2, 2022
…InferenceState)` interface

In #44240 we removed the `CachedMethodTable` support as it turned out to
be ineffective under the current compiler infrastructure.
Because of this, there is no strong reason to keep a method table per `InferenceState`.
This commit simply removes the `method_table(::AbstractInterpreter, ::InferenceState)`
interface and should make it clearer which interface should be overloaded to
implement a contextual dispatch.
staticfloat pushed a commit to JuliaCI/julia-buildkite-testing that referenced this pull request Mar 2, 2022
Since we couldn't confirm any performance benefit from `CachedMethodTable`
in the current infrastructure (see the benchmark results in JuliaLang#44240), now
I'd like to propose to eliminate that entirely and save a bit of space.
aviatesk added a commit that referenced this pull request Mar 3, 2022
…InferenceState)` interface (#44389)

In #44240 we removed the `CachedMethodTable` support as it turned out to
be ineffective under the current compiler infrastructure.
Because of this, there is no strong reason to keep a method table per `InferenceState`.
This commit simply removes the `method_table(::AbstractInterpreter, ::InferenceState)`
interface and should make it clearer which interface should be overloaded to
implement a contextual dispatch.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Since we couldn't confirm any performance benefit from `CachedMethodTable`
in the current infrastructure (see the benchmark results in JuliaLang#44240), now
I'd like to propose to eliminate that entirely and save a bit of space.
@aviatesk aviatesk mentioned this pull request Mar 9, 2022
47 tasks
aviatesk added a commit that referenced this pull request Mar 9, 2022
Since we couldn't confirm any performance benefit from `CachedMethodTable`
in the current infrastructure (see the benchmark results in #44240), now
I'd like to propose to eliminate that entirely and save a bit of space.
aviatesk added a commit that referenced this pull request Mar 9, 2022
…InferenceState)` interface (#44389)

In #44240 we removed the `CachedMethodTable` support as it turned out to
be ineffective under the current compiler infrastructure.
Because of this, there is no strong reason to keep a method table per `InferenceState`.
This commit simply removes the `method_table(::AbstractInterpreter, ::InferenceState)`
interface and should make it clearer which interface should be overloaded to
implement a contextual dispatch.
aviatesk added a commit that referenced this pull request Aug 29, 2022
`CachedMethodTable` was removed within #44240 as we couldn't confirm any
performance improvement then. However it turns out the optimization was
critical in some real world cases (e.g. #46492), so this commit revives
the mechanism with the following tweaks that should make it more effective:
- create method table cache per inference (rather than per local
  inference on a function call as on the previous implementation)
- only use cache mechanism for abstract types (since we already cache
  lookup result at the next level as for concrete types)

As a result, the following snippet reported at #46492 recovers the
compilation performance:
```julia
using ControlSystems
a_2 = [-5 -3; 2 -9]
C_212 = ss(a_2, [1; 2], [1 0; 0 1], [0; 0])
@time norm(C_212)
```

> on master
```
julia> @time norm(C_212)
364.489044 seconds (724.44 M allocations: 92.524 GiB, 6.01% gc time, 100.00% compilation time)
0.5345224838248489
```

> on this commit
```
julia> @time norm(C_212)
 26.539016 seconds (62.09 M allocations: 5.537 GiB, 5.55% gc time, 100.00% compilation time)
0.5345224838248489
```
aviatesk added a commit that referenced this pull request Aug 30, 2022
`CachedMethodTable` was removed within #44240 as we couldn't confirm any
performance improvement then. However it turns out the optimization was
critical in some real world cases (e.g. #46492), so this commit revives
the mechanism with the following tweaks that should make it more effective:
- create method table cache per inference (rather than per local
  inference on a function call as on the previous implementation)
- only use cache mechanism for abstract types (since we already cache
  lookup result at the next level as for concrete types)

As a result, the following snippet reported at #46492 recovers the
compilation performance:
```julia
using ControlSystems
a_2 = [-5 -3; 2 -9]
C_212 = ss(a_2, [1; 2], [1 0; 0 1], [0; 0])
@time norm(C_212)
```

> on master
```
julia> @time norm(C_212)
364.489044 seconds (724.44 M allocations: 92.524 GiB, 6.01% gc time, 100.00% compilation time)
0.5345224838248489
```

> on this commit
```
julia> @time norm(C_212)
 26.539016 seconds (62.09 M allocations: 5.537 GiB, 5.55% gc time, 100.00% compilation time)
0.5345224838248489
```
KristofferC pushed a commit that referenced this pull request Aug 30, 2022
`CachedMethodTable` was removed within #44240 as we couldn't confirm any
performance improvement then. However it turns out the optimization was
critical in some real world cases (e.g. #46492), so this commit revives
the mechanism with the following tweaks that should make it more effective:
- create method table cache per inference (rather than per local
  inference on a function call as on the previous implementation)
- only use cache mechanism for abstract types (since we already cache
  lookup result at the next level as for concrete types)

As a result, the following snippet reported at #46492 recovers the
compilation performance:
```julia
using ControlSystems
a_2 = [-5 -3; 2 -9]
C_212 = ss(a_2, [1; 2], [1 0; 0 1], [0; 0])
@time norm(C_212)
```

> on master
```
julia> @time norm(C_212)
364.489044 seconds (724.44 M allocations: 92.524 GiB, 6.01% gc time, 100.00% compilation time)
0.5345224838248489
```

> on this commit
```
julia> @time norm(C_212)
 26.539016 seconds (62.09 M allocations: 5.537 GiB, 5.55% gc time, 100.00% compilation time)
0.5345224838248489
```

(cherry picked from commit 8445744)
aviatesk added a commit that referenced this pull request Aug 30, 2022
`CachedMethodTable` was removed within #44240 as we couldn't confirm any
performance improvement then. However it turns out the optimization was
critical in some real world cases (e.g. #46492), so this commit revives
the mechanism with the following tweaks that should make it more effective:
- create method table cache per inference (rather than per local
  inference on a function call as on the previous implementation)
- only use cache mechanism for abstract types (since we already cache
  lookup result at the next level as for concrete types)

As a result, the following snippet reported at #46492 recovers the
compilation performance:
```julia
using ControlSystems
a_2 = [-5 -3; 2 -9]
C_212 = ss(a_2, [1; 2], [1 0; 0 1], [0; 0])
@time norm(C_212)
```

> on master
```
julia> @time norm(C_212)
364.489044 seconds (724.44 M allocations: 92.524 GiB, 6.01% gc time, 100.00% compilation time)
0.5345224838248489
```

> on this commit
```
julia> @time norm(C_212)
 26.539016 seconds (62.09 M allocations: 5.537 GiB, 5.55% gc time, 100.00% compilation time)
0.5345224838248489
```
aviatesk added a commit that referenced this pull request Aug 31, 2022
`CachedMethodTable` was removed within #44240 as we couldn't confirm any
performance improvement then. However it turns out the optimization was
critical in some real world cases (e.g. #46492), so this commit revives
the mechanism with the following tweaks that should make it more effective:
- create method table cache per inference (rather than per local
  inference on a function call as on the previous implementation)
- only use cache mechanism for abstract types (since we already cache
  lookup result at the next level as for concrete types)

As a result, the following snippet reported at #46492 recovers the
compilation performance:
```julia
using ControlSystems
a_2 = [-5 -3; 2 -9]
C_212 = ss(a_2, [1; 2], [1 0; 0 1], [0; 0])
@time norm(C_212)
```

> on master
```
julia> @time norm(C_212)
364.489044 seconds (724.44 M allocations: 92.524 GiB, 6.01% gc time, 100.00% compilation time)
0.5345224838248489
```

> on this commit
```
julia> @time norm(C_212)
 26.539016 seconds (62.09 M allocations: 5.537 GiB, 5.55% gc time, 100.00% compilation time)
0.5345224838248489
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants