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

Improve efficiency of NFFT direct transformation in one dimension. #142

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

jenskeiner
Copy link
Contributor

Small change to improve the efficiencvy of the direct NFFT trafo in one dimension:

  • Instead of using memset to initialize the target vector with zeros, it's better to just write the final value to the target in the outer loop. Rationale: Using memset will need to access the entire target vector another time. This can be costly if the vector is large and does not fit inside the CPU cache.
  • Instead of accessing the target location f[j] in the inner loop in each iteration, it may be better to accumulate the value in a local variable and write only the final value to f[j]. Rationale: May reduce potentially slow memory access to f[j], but if f[j] is in the CPU cache and/or the compiler is smart, this may not make any difference.
  • Instead of calling the complex expontential cexp to calculate e^{-i*omega}, use real-valued sin and cos functions. Rationale: cexp supports complex-valued arguments, but the actual argument is always purely imaginary.

It was difficult to test this one because there's not simple benchmark I could quickly run. Also, I tested this on arm64/v8 and cycle.h currently doesn't work for me. So I had to set up a scratch file to run a quick check which is not part of this PR. On my platform, the number of cycles for the direct transform drops to 60-80% compared to before.

Would be good if someone could test this separately and on a different architecture (e.g. amd64) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need the explicit casts or tests won't compile for me.

@michaelquellmalz
Copy link
Member

Looks good. I also had about 20 % speed increase on my Intel 10700 CPU with GCC10 and time measured in Matlab.

I was wondering why you did the changes only in 1D, because these improvements should also work in the multivariate case as well as for the direct adjoint NFFT.

@jenskeiner
Copy link
Contributor Author

I'll make more changes, for d > 1 and possibly the adjoint transform as well. I was just going to get it in step by step so everything can be reasonably tested. There's no rush to merge this PR from my side. But I don't know how fast I'll be able to get to make more changes, so it could make sense to merge this, and then I'll just open new PRs for the rest.

@michaelquellmalz michaelquellmalz added this to the 3.5.4 milestone Oct 15, 2024
@michaelquellmalz michaelquellmalz merged commit f9e1bde into develop Oct 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants