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

Make work on IRTools #37

Closed
wants to merge 1 commit into from
Closed

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented May 28, 2019

This swaps out Cassette for IRTools @dynamo.

Which was an attempt to solve #35 which it does.
Time of first call for SVD is down to about 1/4 what it was;

Original, Cassette

3.128563 seconds (11.08 M allocations: 577.564 MiB
0.005609 seconds (2.19 k allocations: 152.170 KiB)

No Overdubs (Best Case Senario)

0.794261 seconds (2.49 M allocations: 120.038 MiB, 7.86% gc time)
0.000015 seconds (23 allocations: 3.641 KiB)

IRTools, with precompile.jl (This branch)

0.881375 seconds (2.98 M allocations: 146.369 MiB, 6.39% gc time)
0.000031 seconds (33 allocations: 4.188 KiB)

Note however I added the precompile.jl which makes sure that IRTools itself has been compiled before anything else. Without that it is much worse than anything else.

Next I will try that for Cassette.

Commented out is a sketch of what it should looklike without usiing IRTools.IR
which is close to what it should looklike without IRTools at all.
Doesn't quiet work.
Which might be nice since no IRTools dep, no need to precompile IRTools.
This is a really simple transform.

@oxinabox
Copy link
Member Author

oxinabox commented May 28, 2019

Nope, Cassette does not do so well when I add the precompile

Cassette with precompile.jl

2.838656 seconds (11.13 M allocations: 580.525 MiB, 7.72% gc time)
0.004810 seconds (2.19 k allocations: 152.170 KiB)

@oxinabox oxinabox changed the title Make work on IRTools] Make work on IRTools May 28, 2019
@ararslan
Copy link
Member

According to @jrevels, the use of Cassette in this package is purely an implementation detail and could be replaced with regular ol' multiple dispatch. The reason why Jarrett didn't do that to begin with is it requires defining a fair number of methods to resolve ambiguities between the various differential types, rule types, etc. I think going that route will be a cleaner solution overall than replacing the dependency on Cassette with on one IRTools.

@oxinabox
Copy link
Member Author

We might want to do this first, which makes things faster.
Since it will take/is a whole ton less code than #38 will be

@jrevels
Copy link
Member

jrevels commented May 29, 2019

Do we know what the reason for the overhead here is and why switching away from Cassette fixes it

@oxinabox
Copy link
Member Author

I do not know.

@jrevels
Copy link
Member

jrevels commented May 29, 2019

Would probably be good to debug the actual perf issue before churning through a lot of code. Would also be interested if performing the transform via a Cassette pass vs. using dispatch improved things.

@oxinabox
Copy link
Member Author

oxinabox commented May 29, 2019

Debugging Cassette performance issues is really really annoying.
Even more so when it is related to the compile cache, since that means restarting Julia between runs.
To which ever poor sap tries to do it. (probably future me)

StatsProfile.jl and Cthulu.jl are best tools

@willtebbutt
Copy link
Member

Should we anticipate that a custom Cassette @pass that performs the same transformation as @dynamo would result in the same performance gains? i.e. is doing contextual dispatch (and leaving it up to the compiler to figure out that overdub does nothing both of the time), rather than manual IR re-writing, the thing that's degrading performance here?

@oxinabox
Copy link
Member Author

resolved by JuliaDiff/ChainRulesCore.jl#35

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.

4 participants