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

Use Julia 1.6's new reinterpretarray machinery #145

Merged
merged 2 commits into from
Oct 8, 2020
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Oct 7, 2020

When available, this switches to using Julia 1.6's new reinterpret(reshape, T, A) for our colorview/channelview functionality. As documented in #142, this essentially puts to rest the performance problems we've had since Julia 1.0 with ReinterpretArray.

The tests will still not quite pass on nightly due to show failures, but I plan to fix that in a separate PR.

Closes #142

@codecov
Copy link

codecov bot commented Oct 7, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@d9dccd9). Click here to learn what that means.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #145   +/-   ##
=========================================
  Coverage          ?   74.41%           
=========================================
  Files             ?       10           
  Lines             ?      516           
  Branches          ?        0           
=========================================
  Hits              ?      384           
  Misses            ?      132           
  Partials          ?        0           
Impacted Files Coverage Δ
src/ImageCore.jl 70.37% <ø> (ø)
src/colorchannels.jl 43.57% <45.45%> (ø)
src/convert_reinterpret.jl 85.36% <80.64%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9dccd9...9ab9089. Read the comment docs.

@timholy
Copy link
Member Author

timholy commented Oct 7, 2020

So, it seems that our benchmarks don't run anymore as part of CI. Intended? I see #123.

I should also acknowledge that because I've tightened the "bounds" on what we consider acceptable, include("benchmarks.jl") is pretty noisy on 1.5:

julia> include("benchmarks.jl")
[ Info: Benchmark tests are warnings for now
┌ Warning: colorview2: failed on mysum_elt_boundscheck with eltype Float32, time ratio 8.859957776213934, tol 5
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: colorview2: failed on mysum_elt_inbounds with eltype Float32, time ratio 8.861587413823582, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: channelview1: failed on mysum_index_inbounds_simd with eltype Float32, time ratio 27.78320479862896, tol 20
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:119
┌ Warning: colorview2: failed on mysum_index_inbounds_simd with eltype Float32, time ratio 6.7447698744769875, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: colorview1: failed on myfill1! with eltype Float32, time ratio 11.369456762188495, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:132
┌ Warning: colorview2: failed on myfill1! with eltype Float32, time ratio 18.31693755262115, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: channelview1: failed on myfill2! with eltype Float32, time ratio 29.777311566200062, tol 20
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:119
┌ Warning: colorview1: failed on myfill2! with eltype Float32, time ratio 13.63551190747891, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:132
┌ Warning: colorview2: failed on myfill2! with eltype Float32, time ratio 12.960752632917918, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: colorview2: failed on mysum_elt_boundscheck with eltype Float64, time ratio 7.129490249743414, tol 5
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: colorview2: failed on mysum_elt_inbounds with eltype Float64, time ratio 7.149125814192664, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: colorview1: failed on myfill1! with eltype Float64, time ratio 7.043604325936975, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:132
┌ Warning: colorview2: failed on myfill1! with eltype Float64, time ratio 9.796395232297128, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138
┌ Warning: colorview1: failed on myfill2! with eltype Float64, time ratio 6.146765267631468, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:132
┌ Warning: colorview2: failed on myfill2! with eltype Float64, time ratio 9.315949234411704, tol 3
└ @ Main ~/.julia/dev/ImageCore/test/benchmarks.jl:138

but on 1.6 I get

julia> include("benchmarks.jl")
[ Info: Benchmark tests are warnings for now

julia>

@johnnychen94
Copy link
Member

I'm a little unsure what to benchmark so #123 is probably abandoned. The noise you mentioned is also concerning that I failed to restart that...

FYI, https://github.com/JuliaImages/ImageContrastAdjustment.jl has a well-running benchmark CI that you can use to get some idea.

@timholy
Copy link
Member Author

timholy commented Oct 7, 2020

For now, though, should we still be running the benchmarks? I'm guessing that in

# run these last
isCI = haskey(ENV, "CI") || get(ENV, "JULIA_PKGEVAL", false)
if Base.JLOptions().can_inline == 1 && !isCI
@info "running benchmarks"
include("benchmarks.jl") # these fail if inlining is off
end
"CI" isn't set by GitHub Actions?

EDIT: oh wait, we specifically don't run them on CI. Can you remind me why?
EDIT2: nvm, see #12 and specifically 623052b. I don't know why, maybe Travis was too noisy? Should we try again with GitHub Actions?

@johnnychen94
Copy link
Member

johnnychen94 commented Oct 7, 2020

One drawback of setting up benchmarkCI is that we hand almost everything to PkgBenchmark and so we can't do

t_baseline = @belapsed f_baseline(...)
t = @belapsed f(...)
ratio = t/t_baseline

we might need two benchmark CIs for the very different two benchmark tasks:

  • master branch vs PR branch (using BenchmarkCI)
  • baseline indexing vs reinterpret indexing (set up our own version from test/benchmark.jl)

@timholy
Copy link
Member Author

timholy commented Oct 7, 2020

Gotcha. Seems complicated enough that we should just accept the status quo here and deal with it separately.

I'll leave this open for a bit for review, in case there are any concerns.

@timholy timholy merged commit 02593d9 into master Oct 8, 2020
@timholy timholy deleted the teh/reinterpretarray branch October 8, 2020 10:16
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.

2 participants