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

ManyPencilArraysRFFT!: avoid inefficient view + reshape #75

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

fbignonnet
Copy link
Contributor

@fbignonnet fbignonnet commented Sep 25, 2024

Hi,

I propose a minor update of multiarrays_r2c.jl to fix two issues for the use of the view to the first (real valued) PencilArray of the ManyPencilArraysRFFT!.

Since the input real valued array of a RFFT uses less memory than the complex output, the inplace RFFT! requires that the view to the real valued array is reshaped from a vector SubArray vec of some larger vector data. This was previously done using vec = view(data, Base.OneTo(n)).

However reshape(vec, dims) outputs a Base.ReshapedArray{T, N, SubArray{T, 1, Vector{T}, Tuple{UnitRange{Int64}}, true}, Tuple{}}, for which the compiler may produce inefficient code.

The proposed alternative is to use vec = unsafe_wrap(typeof(data), pointer(data), n), after which reshape(vec, dims) outputs a simple Array{T,N}.

This fixes:

  1. Some performance issues for the use of the real valued PencilArray view of the ManyPencilArraysRFFT!
  2. Error while saving the real valued PencilArray view using PencilArrays.PencilIO

The same idea could be used in function _make_arrays of multiarrays.jl of PencilArrays, line 137: vec = view(data, Base.OneTo(n)).

Fix efficiency of view to real data vector using unsafe_wrap
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.97%. Comparing base (0429e73) to head (fb0f12a).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   96.81%   96.97%   +0.15%     
==========================================
  Files          11       11              
  Lines         534      529       -5     
==========================================
- Hits          517      513       -4     
+ Misses         17       16       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jipolanco
Copy link
Owner

Looks good, thanks!

Note that we already use the same trick in PencilArrays transpositions (here.

And I agree, I'll do the same for ManyPencilArray in PencilArrays.

@jipolanco jipolanco merged commit 0d899a9 into jipolanco:master Sep 25, 2024
5 of 6 checks passed
@jipolanco jipolanco changed the title Update multiarrays_r2c.jl ManyPencilArraysRFFT!: avoid inefficient view + reshape Sep 25, 2024
@fbignonnet
Copy link
Contributor Author

Great, thanks!

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