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

Dynamically disable CuArray scalar operations #851

Merged
merged 8 commits into from
Aug 12, 2020

Conversation

ali-ramadhan
Copy link
Member

Got bitten by CuArray scalar operations while debugging some Europa runs so this PR adds an __init__() function to the Oceananigans module that disables CuArray scalar operations once the module has been loaded and only allows scalar ops via the CUDA.@allowscalar macro.

Now when we accidentally perform scalar ops on CuArrays we should get an error, which is much better than having silent performance regressions which has happened multiple times in the past.

Two remaining problems:

  1. set!(u::Field, v::Number) = @. u.data = v actually incurs scalar ops because a::CuArray .= 1.5 does not but a::OffsetArray{CuArray .= 1.5 does, so it's probably something we have to fix by adapting broadcasting over offsetarrays?
  2. For similar broadcasting reasons I think
    data = cpudata(output)
    ds[name][:, :, :, time_index] = view(data, ow.slices[name]...)
    also incurs scalar ops during NetCDF output so I will try changing back to the old behavior where we used something like `
    data = interiorparent(output)
    !isa(output, Array) && (data = Array(data))
    ds[name][:, :, :, time_index] = view(data, ow.slices[name]...)

Resolves #276

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Aug 12, 2020

Ah so the dispatch on

@hascuda @inline cpudata(f::Field{X, Y, Z, <:CuArray}) where {X, Y, Z} =
    OffsetArray(Array(parent(f)), f.grid, location(f))

is wrong so it falls back on cpudata(a) = a, it should be something like

Field{X, Y, Z, <:OffsetArray{T, D, <:CuArray} where {T, D}} where {X, Y, Z}

but still trying to get it to work.

Will add cpudata tests when I figure it out.

src/Fields/set!.jl Outdated Show resolved Hide resolved
test/test_halo_regions.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #851 into master will decrease coverage by 0.09%.
The diff coverage is 67.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #851      +/-   ##
==========================================
- Coverage   72.62%   72.52%   -0.10%     
==========================================
  Files         188      187       -1     
  Lines        5337     5322      -15     
==========================================
- Hits         3876     3860      -16     
- Misses       1461     1462       +1     
Impacted Files Coverage Δ
src/OutputWriters/OutputWriters.jl 100.00% <ø> (ø)
src/Solvers/channel_pressure_solver.jl 31.08% <0.00%> (ø)
...c/Solvers/horizontally_periodic_pressure_solver.jl 42.59% <0.00%> (ø)
src/Fields/field.jl 69.84% <66.66%> (ø)
test/test_output_writers.jl 91.59% <69.44%> (-0.20%) ⬇️
test/test_fields.jl 95.00% <85.71%> (+0.35%) ⬆️
src/Fields/set!.jl 36.36% <100.00%> (ø)
src/Oceananigans.jl 100.00% <100.00%> (ø)
test/test_halo_regions.jl 91.66% <100.00%> (ø)

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 a8c35f5...a30e597. Read the comment docs.

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.

Dynamically disable CuArray scalar operations
2 participants