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

Fix a second place where particles compute the size of a buffer. #16815

Merged
merged 1 commit into from Mar 30, 2024

Conversation

bangerth
Copy link
Member

Like #16813. This fixes the remaining 5 of the 16 particle-related failures mentioned in #16796.

It turns out that we have duplicate code to compute the size of the buffer, in class Particle and class ParticleAccessor. #16813 fixes the first of these, this patch the second. This is perhaps not the best design, but it is what it is and my interest for the moment is just fixing the bug.

While there, I'm also adding an assertion I found useful in debugging.

@bangerth
Copy link
Member Author

@gassmoeller @blaisb FYI

@kronbichler
Copy link
Member

/__w/dealii/dealii/source/particles/particle_handler.cc:2371:25: error: variable ‘x’ set but not used [-Werror=unused-but-set-variable]
 2371 |             const auto  x = insert_particle(data, cell_to_store_particles);

@gassmoeller
Copy link
Member

Sorry I just got to properly think about the change of aligning the Tensor class. But I have a question: If Point<dim> now needs more space in order to be aligned, does that mean that any algorithm that iterates over a std::vector<Point<dim>> (like the one in property_pool.h) now requires more memory bandwidth (or is slower for the same memory bandwidth)? Because some of the algorithms for particles may be bandwidth limited (e.g. updating particle positions). I have not measured this, so I cannot say if it is actually the case. Even if it is, the change may of course still be worth it for a lot of other applications of the Tensor class, but then I would need to redo a few measurements on the performance (I just re-submitted a paper with performance measurements for particle advection).

@kronbichler
Copy link
Member

does that mean that any algorithm that iterates over a std::vector<Point> (like the one in property_pool.h) now requires more memory bandwidth (or is slower for the same memory bandwidth)?

It is clear that a standard Point<3> will now involve memory traffic of 32 bytes per point rather than 24 bytes because the CPU loads full cache lines of 64 bytes. So it is quite likely that you see some slowdown in case the bandwidth is the limit). I think we should carefully assess such a scenario; do you have something where you can measure it immediately, or something to share to see how things behave (and if we might possibly need to think of alternatives)?

@gassmoeller
Copy link
Member

Let me think about that, I have some benchmarks, but they are mostly 2D which shouldnt change, I will look for a 3D case. This should not hold up this PR, which is a clear fix. I will reference my comment in #16796, which is really the place where I should have made this comment.

@tamiko tamiko merged commit ff1ac3b into dealii:master Mar 30, 2024
16 checks passed
@bangerth bangerth deleted the fix-particle-size-2 branch March 31, 2024 00:11
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.

None yet

4 participants