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

Remove dynamic allocations from swarm particle creation #996

Merged
merged 21 commits into from
Feb 13, 2024

Conversation

brryan
Copy link
Collaborator

@brryan brryan commented Jan 19, 2024

PR Summary

Swarms previously created multiple ParArrays during the AddEmptyParticles call to create new particles. This PR pre-allocates that memory during resize operations and returns an accessor to new particle indices, resolving that issue.

This necessarily breaks previous usage of the AddEmptyParticles function and will affect all downstream codes using particles.

API change:

ParArray1D<bool> Swarm::AddEmptyParticles(const int num_to_add, ParArrayND<int> &new_indices)

is now

NewParticlesContext Swarm::AddEmptyParticles(const int num_to_add)

where NewParticlesContext adds the methods int GetNewParticlesMaxIndex() that returns the max index of the new particles -> swarm index map, and int GetNewParticleIndex(const int n) converts a new particle index to the swarm index.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@brryan brryan changed the title Remove dynamic allocations from swarm particle creation WIP: Remove dynamic allocations from swarm particle creation Jan 19, 2024
@brryan brryan changed the title WIP: Remove dynamic allocations from swarm particle creation Remove dynamic allocations from swarm particle creation Jan 19, 2024
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Overall this seems like a pretty minor change, but good for performance. And I like the new API, which feels cleaner. 👍

example/particle_leapfrog/particle_leapfrog.cpp Outdated Show resolved Hide resolved
src/interface/swarm.hpp Show resolved Hide resolved
@brryan
Copy link
Collaborator Author

brryan commented Jan 19, 2024

Alright this is ready for review/merge

@Yurlungur
Copy link
Collaborator

@pgrete or @bprather maybe one of you can review?

Copy link
Collaborator

@bprather bprather left a comment

Choose a reason for hiding this comment

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

LGTM. I'm not far enough into particles to be annoyed by the slight interface change, and this is clean and easy enough to understand with the example.

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

LGTM and the changes make sense from a performance point of view.
The main snarkyke case-y comment I have why you chose you use camel case (and even refactor some existing variables) versus snake case.
I'm general in favor of consistent style across the code base.

example/particle_leapfrog/particle_leapfrog.cpp Outdated Show resolved Hide resolved
example/particles/particles.cpp Outdated Show resolved Hide resolved
src/interface/swarm.cpp Show resolved Hide resolved
src/interface/swarm.cpp Outdated Show resolved Hide resolved
src/interface/swarm.cpp Outdated Show resolved Hide resolved
src/interface/swarm.cpp Show resolved Hide resolved
src/interface/swarm.cpp Outdated Show resolved Hide resolved
@brryan
Copy link
Collaborator Author

brryan commented Jan 30, 2024

LGTM and the changes make sense from a performance point of view. The main snarkyke case-y comment I have why you chose you use camel case (and even refactor some existing variables) versus snake case. I'm general in favor of consistent style across the code base.

@pgrete You are absolutely right about the case style issue with variable names, I was mistakenly taking this in the wrong direction. Thanks for pointing that out. If I didnt miss anything, I've switched every variable in swarm over to camel case. This should be ready for merge now if you're happy

@brryan
Copy link
Collaborator Author

brryan commented Feb 13, 2024

Alright all concerns look to be addressed to me so setting this to automerge, thanks all for reviews.

@brryan brryan enabled auto-merge (squash) February 13, 2024 17:12
@brryan brryan disabled auto-merge February 13, 2024 20:26
@brryan brryan enabled auto-merge (squash) February 13, 2024 20:26
@lroberts36 lroberts36 changed the title Remove dynamic allocations from swarm particle creation WIP: Remove dynamic allocations from swarm particle creation Feb 13, 2024
@lroberts36 lroberts36 changed the title WIP: Remove dynamic allocations from swarm particle creation Remove dynamic allocations from swarm particle creation Feb 13, 2024
@brryan brryan merged commit 1eb1645 into develop Feb 13, 2024
52 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.

4 participants