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

Minimal benchmark setup #736

Merged
merged 3 commits into from
Feb 19, 2020
Merged

Minimal benchmark setup #736

merged 3 commits into from
Feb 19, 2020

Conversation

tkf
Copy link
Member

@tkf tkf commented Feb 18, 2020

See #737. Please comment on how we organize benchmark/* here.

@tkf tkf mentioned this pull request Feb 18, 2020
2 tasks
@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.975% when pulling c511e52 on tkf/pkgbenchmark into 7a78efd on master.

@@ -0,0 +1,12 @@
# Each file of the form "bench_$(name).jl" in this directory is `include`d and
# its last statement is assumed to be a `BenchmarkGroup`. This group is added
# to the top-level group `SUITE` with the `$name` extracted from the file name.
Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about this strategy? I use this for all my packages and it works well for me. This is useful for running each file separately like this: run(include("benchmark/bench_qr.jl")).

Copy link
Member

Choose a reason for hiding this comment

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

This sounds completely fine to me!


PS: My fantasy interface might be more akin to @testset where the groups are assembled "magically" using dynamic scope

@benchmark_group "all" begin
    @benchmark_group "QR $K" for K=1:22
        a = rand(SMatrix{K,K,Float64,K*K})
        m = Matrix(a)
        @benchmarkable qr($a)
        @benchmarkable qr($m)
     end
     # other groups  or includes()...
end

but unlike Test, to allow running these selectively. But no doubt that's an issue for BenchmarkTools ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

PS: My fantasy interface might be more akin to @testset where the groups are assembled "magically" using dynamic scope

Ref JuliaCI/PkgBenchmark.jl#9 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the crossref 👍

I do strongly disagree with Jarret on that one. I think dynamic scoping (not global scope mind you!) can be fairly clear and non-magical for this kind of use as long as it's clearly documented.

@tkf tkf marked this pull request as ready for review February 19, 2020 01:52
Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

I think this is good; simple enough and flexible.

@@ -0,0 +1,12 @@
# Each file of the form "bench_$(name).jl" in this directory is `include`d and
# its last statement is assumed to be a `BenchmarkGroup`. This group is added
# to the top-level group `SUITE` with the `$name` extracted from the file name.
Copy link
Member

Choose a reason for hiding this comment

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

This sounds completely fine to me!


PS: My fantasy interface might be more akin to @testset where the groups are assembled "magically" using dynamic scope

@benchmark_group "all" begin
    @benchmark_group "QR $K" for K=1:22
        a = rand(SMatrix{K,K,Float64,K*K})
        m = Matrix(a)
        @benchmarkable qr($a)
        @benchmarkable qr($m)
     end
     # other groups  or includes()...
end

but unlike Test, to allow running these selectively. But no doubt that's an issue for BenchmarkTools ;-)

@tkf tkf merged commit d317933 into master Feb 19, 2020
@tkf tkf deleted the tkf/pkgbenchmark branch February 19, 2020 07:55
# to the top-level group `SUITE` with the `$name` extracted from the file name.

using BenchmarkTools
SUITE = BenchmarkGroup()
Copy link
Contributor

Choose a reason for hiding this comment

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

could be const I guess?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, initially I didn't want const to avoid seeing the redefinition warning but it's not a good motivation anymore since I use modules.

using LinearAlgebra
using StaticArrays

suite = BenchmarkGroup()
Copy link
Contributor

Choose a reason for hiding this comment

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

could be const I guess?

@tkf tkf mentioned this pull request Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants