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

Minor optimization: use convert rather than constructor to convert array type before output #1182

Closed
glwagner opened this issue Nov 14, 2020 · 1 comment
Labels
output 💾 performance 🏍️ So we can get the wrong answer even faster

Comments

@glwagner
Copy link
Member

glwagner commented Nov 14, 2020

Currently, we use the constructor output_writer.array_type to convert array data prior to outputting:

convert_output(output::AbstractArray, writer) = CUDA.@allowscalar writer.array_type(output)

But we should use convert instead, because this avoids allocating memory when no type conversion is needed:

julia> a = rand(1, 1, 1)
1×1×1 Array{Float64,3}:
[:, :, 1] =
 0.7727202498256802

julia> b = convert(Array{Float64}, a)
1×1×1 Array{Float64,3}:
[:, :, 1] =
 0.7727202498256802

julia> b === a
true

so b is just a reference to a, but

julia> c = Array{Float64}(a)
1×1×1 Array{Float64,3}:
[:, :, 1] =
 0.7727202498256802

julia> c === a
false

c is not a reference to a.

This matters very little since we basically always need to allocate memory to convert from Float64 to Float32.

A related minor optimization would be to avoid converting views to the same type as the parent array, since we could just output those directly. I think we have to use dispatch on one of the type parameters of SubArray for that. But maybe simpler code is worth not implementing a minor optimization for an edge case.

@navidcy navidcy added output 💾 performance 🏍️ So we can get the wrong answer even faster labels Nov 16, 2020
@glwagner
Copy link
Member Author

I'm closing this issue because I'm judging that it's not of current, timely relevance to Oceananigans development. If you would like to make it a higher priority or if you think the issue was closed in error please feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
output 💾 performance 🏍️ So we can get the wrong answer even faster
Projects
None yet
Development

No branches or pull requests

2 participants