Skip to content

Trying some other optimizations beside type-infer #130

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

Merged
merged 15 commits into from
Mar 18, 2022
Merged

Trying some other optimizations beside type-infer #130

merged 15 commits into from
Mar 18, 2022

Conversation

KDr2
Copy link
Member

@KDr2 KDr2 commented Mar 5, 2022

No description provided.

@KDr2 KDr2 requested a review from rikhuijzer March 5, 2022 00:28
@KDr2
Copy link
Member Author

KDr2 commented Mar 5, 2022

@testset "Turing" begin
    @time include("inference/AdvancedSMC.jl")
    @time include("inference/gibbs.jl")
    @time include("contrib/inference/sghmc.jl")
    @time include("stdlib/RandomMeasures.jl")
end

Branch master:

 34.395940 seconds (82.78 M allocations: 5.174 GiB, 4.03% gc time, 95.12% compilation time)
285.165249 seconds (834.03 M allocations: 67.579 GiB, 3.62% gc time, 25.78% compilation time)
 11.141108 seconds (38.60 M allocations: 2.341 GiB, 4.64% gc time, 98.42% compilation time)
  7.159114 seconds (16.94 M allocations: 1.029 GiB, 4.85% gc time, 70.14% compilation time)

Branch no-type-opt:

 39.743907 seconds (89.76 M allocations: 5.518 GiB, 3.81% gc time, 95.43% compilation time)
299.946079 seconds (831.72 M allocations: 67.493 GiB, 3.45% gc time, 24.85% compilation time)
 11.235855 seconds (38.59 M allocations: 2.341 GiB, 4.98% gc time, 97.71% compilation time)
  9.119536 seconds (26.03 M allocations: 1.275 GiB, 4.52% gc time, 75.07% compilation time)

It becomes slower... 😢

@KDr2
Copy link
Member Author

KDr2 commented Mar 5, 2022

First commit(5ac43d4) on Branch no-type-opt:

 34.459184 seconds (81.37 M allocations: 5.088 GiB, 3.99% gc time, 94.95% compilation time)
302.663785 seconds (553.19 M allocations: 59.879 GiB, 3.18% gc time, 23.32% compilation time)
 10.996351 seconds (38.60 M allocations: 2.341 GiB, 5.07% gc time, 98.22% compilation time)
  7.437808 seconds (15.85 M allocations: 1012.160 MiB, 4.32% gc time, 69.07% compilation time)

@KDr2
Copy link
Member Author

KDr2 commented Mar 5, 2022

Second commit(cce6cb3) on Branch no-type-opt:

 34.994963 seconds (82.41 M allocations: 5.142 GiB, 4.03% gc time, 95.08% compilation time)
300.684379 seconds (615.39 M allocations: 62.235 GiB, 3.44% gc time, 23.27% compilation time)
 10.994031 seconds (38.60 M allocations: 2.341 GiB, 4.99% gc time, 98.45% compilation time)
  7.422173 seconds (17.95 M allocations: 1.065 GiB, 4.46% gc time, 69.01% compilation time)

@KDr2
Copy link
Member Author

KDr2 commented Mar 5, 2022

First commit(5ac43d4) on Branch no-type-opt:

This commit has the least memory allocation, but it doesn't save much time cost.

@rikhuijzer
Copy link
Contributor

First commit(5ac43d4) on Branch no-type-opt:

This commit has the least memory allocation, but it doesn't save much time cost.

Could be that your computer was more busy during the one run versus the other. What you can do to dive deeper into why things are going faster or slower is to put @time things at some places in the codebase. As far as I can tell, it's difficult to see these instruction constructors as stand-alone, so then running larger parts with a @time does help sometimes.

Copy link
Contributor

@rikhuijzer rikhuijzer left a comment

Choose a reason for hiding this comment

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

I don't understand everything. Some suggestions.

Good news about the benchmarking: TuringLang/Turing.jl#1787 is merged, so we can now see the allocations in the integration test.

@KDr2 KDr2 marked this pull request as ready for review March 5, 2022 12:12
@yebai
Copy link
Member

yebai commented Mar 17, 2022

@KDr2 Maybe we should introduce a few benchmark examples that do not depend on Turing. It allows us to better understand the performance bottleneck -- the Turing integration tests are helpful, but it involves many other parts, which make the runtime numbers hard to analyse.

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, @KDr2 - it looks good to me. I left some minor comments below.

I think we still need some benchmarks before merging. We need to ensure each Instruction in a TapedFunction is type-sable, whenever the original function f underlying TapedFunction is type-stabe.

@yebai yebai requested a review from devmotion March 17, 2022 17:29
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

My main suggestion is to use Base.RefValue instead of Ref.

It is unclear to me if/how this affects performance. It would be good to perform some benchmarks (or (re-)post them, if they exist - I assumed the comments above are outdated).

@yebai yebai merged commit eacff8c into master Mar 18, 2022
@delete-merged-branch delete-merged-branch bot deleted the no-type-opt branch March 18, 2022 13:48
@yebai
Copy link
Member

yebai commented Mar 18, 2022

many thanks @KDr2!

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