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

Avoid some uses of Triangulation::user_flags #13853

Merged
merged 1 commit into from Jun 24, 2022

Conversation

kronbichler
Copy link
Member

While working on the internal restructuring of DoFHandler, and also when looking at performance profiles, I observed an annoyingly high number of calls to Triangulation::save_user_flags and Triangulation::load_user_flags. We used those in many places to store some information regarding the lines or faces of a triangulation we only want to visit once, e.g. during the enumeration of the DoFs. However, that comes with a lot of boilerplate, because we always need to save the user flags, do our thing, and restore them. Also from a performance point of view, these operations are not for free, because std::vector<bool> has no single-instruction access function.

This PR cleans up the uses outside the immediate grid constructions that should be safe: Keeping a separate vector with the flags communicates the intent much more clearly, and avoids going back and forth between a user's flags, thus reducing the time in those setup functions. A net of 270 reduced lines of code shows that this is a significant simplification. (OK, 20 lines or so are due to range-based for loops I changed on the way).

I tagged this with the 9.4 milestone because I want to get it done, but it is not an urgent PR.

@bangerth
Copy link
Member

This is the sort of thing that makes me nervous just before a release, at least the parts that aren't in dof_handler.cc and tria.cc. I think the patch is good, but since it does not provide functionality we didn't have before my vote would be to let the patch wait until after we create the branch. Do you insist on having it in 9.4? If you do, could you break it into those parts that are definitely tested in hundreds of tests (e.g., in DoFHandler::reserve_*()) which I'd be ok to merge, and those parts that maybe don't get run in half of our tests?

@kronbichler kronbichler modified the milestones: Release 9.4, Release 9.5 May 30, 2022
@kronbichler
Copy link
Member Author

kronbichler commented May 30, 2022

I agree, the interference with other PRs should be small enough to not have to look into it again, so let's postpone to 9.5.

@kronbichler
Copy link
Member Author

For the record, while creating the patch, the combination of hp and multigrid tests that I ran to verify did uncover three places where I had made a mistake, so we seem to indeed have covered most of these places reasonably well.

source/dofs/dof_handler.cc Outdated Show resolved Hide resolved
@kronbichler
Copy link
Member Author

For reference, this PR will have significant effect on our performance tests. The timing_step37, when I run it on 1 node / 72 MPI ranks, I record 0.77 seconds for the subtest setup_dofs on top of this PR and #13828, compared to 1.00 seconds on current master. We do regress compared to the beginning of May since the benchmark added DoF renumbering function via #13674 (when the time was around 0.4 seconds), but we are significantly ahead of the 0.94 seconds we had before I started my cleanups in DoFHandler and related two weeks ago.

Regarding larger scales, we are significantly ahead already before this PR: On 64 nodes / 4608 MPI ranks, the create_grid function went from 0.22 seconds to 0.11 seconds on an average of 20 samples, and the setup_dofs function went from 0.062 to 0.055 seconds (so despite being slower on 1 node than we were in mid May; we slightly improve on this number, too, despite this being inside the scaling limit).

@drwells drwells merged commit bff0a1f into dealii:master Jun 24, 2022
@kronbichler kronbichler deleted the avoid_user_flag branch July 4, 2022 21:05
mkghadban pushed a commit to OpenFCST/dealii that referenced this pull request Sep 8, 2022
Avoid some uses of Triangulation::user_flags
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

3 participants