-
Notifications
You must be signed in to change notification settings - Fork 43
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
Broadcast benchmarks #30
Conversation
@jrevels, it seems like there is a |
It's not necessary for you to update the JLD file. The failure is due to JuliaCI/BenchmarkTools.jl#23. That PR shouldn't have affected BaseBenchmarks yet, since I haven't tagged it in a release, but the Travis script in this repo clones BenchmarkTools directly instead of using the latest stable release (a holdover from before BenchmarkTools was registered). I'll open a PR to fix the Travis script, and rerun the tests here. |
The Travis fix is merged, if you'd like to rebase and rerun the tests here. |
Okay, thanks. |
R = Array(Float64, length(x),length(y)) | ||
r = similar(z) | ||
|
||
g["fusion", "Float64", size(r), 1] = @benchmarkable perf_bcast!($r, $z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is, the fully qualified ID for this benchmark will be ["broadcast", "fusion", ("fusion", "Float64", size(R), 1)]
. Can we remove the extra "fusion"
(same goes for the other benchmarks where the group name is duplicated in the local key)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Tests are green; any other comments? |
perf_op_bcast!(r, x) = @dotcompat r .= 3 .* x .- 4 .* x.^2 .+ x .* x .- x .^ 3 | ||
perf_op_bcast!(R, x, y) = @dotcompat R .= 3 .* x .- 4 .* y.^2 .+ x .* y .- x .^ 3 | ||
|
||
g["dotop", "Float64", size(r), 1] = @benchmarkable perf_op_bcast!($r, $z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dotop"
is redundant here for the reasons previously mentioned.
BTW, apologies for the slow review. I've been hiking on vacation, but just got back, so things should move a bit more quickly now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs something to distinguish it from the other benchmarks in the fusion
group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, it's already in a different group (the "dotop"
group as defined on line 50). The difference in the fully qualified IDs is thus the group name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, right, I missed that addgroup
line
* feed quasiquote variables in as arguments interpolating these into the AST gives the optimizer too much flexibility for a benchmark, that can mean the compiler will be able to just constant-fold away the work * handle case where core isn't and Expr not sure what it means to benchmark a constant value, but need to handle this case anyways
This adds a new suite (
broadcast
) of benchmarks testing the new broadcast-fusion functionality, the upcoming dot-operator=broadcast change, and broadcast on sparse vectors, for JuliaLang/julia#16285.