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

Pure SoA Particle: Separate Array for IdCPU #3585

Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Oct 6, 2023

Summary

This addresses a regression we see when moving to pure SoA particles:

  • slightly slower read/write to Ids when needed, e.g., for sorting
  • issues going up to the full 64bit range

Additional background

Once finished, this will close #3569.

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@AlexanderSinn
Copy link
Member

It would be nice if there was some sort of option to disable the 64-bit ID to save memory in the cases where a unique id for each particle is not required (i.e., 1 bit per particle in some other component is sufficient to track valid/invalid). It might be possible to “hack” this outside of amrex by setting the allocator of the ID PODVector to a null allocator.

@ax3l
Copy link
Member Author

ax3l commented Nov 27, 2023

Hi @AlexanderSinn, thank you for the comment.

On first thought, I am not sure this small optimization is worth the additional compile time that this will cause in pyAMReX, ABLASTR, AMReX and HiPACE++ for differentially specialized PC template classes.

What performance benefits do you see from this? Are the species that never need tracking very large and will significantly save memory without it?

@AlexanderSinn
Copy link
Member

For the plasma in HiPACE++ there is no per-particle diagnostics as it would be too much data, so there is no possibility to use the id for particle tracking. Besides the id the plasma has two other integer components so we could put those inside a 64 bit id.

The beam in HiPACE++ does have per particle diagnostics and can be interesting for particle tracking, however only if the per particle diagnostics is actually used. It is possible to have multiple beams that are each a few terabytes in size of which the 64bit id would be 12.5%. This is too much data for diagnostics/tracking, but the id still has to be communicated. Technically because the communication in HiPACE++ is custom, we don’t have to communicate every component that is in the particle container if we add a special case.

@ax3l
Copy link
Member Author

ax3l commented Nov 27, 2023

Thank you for the details. I am not yet fully convinced if this is not a premature optimization, because for background plasmas (in WarpX), filtering diagnostics can be essential to statistically analyze representative particles. The same can be true in the future for HiPACE++ for specific research tasks. But let's chat more tomorrow in the dev meeting about it.

In general, if we can easily make this a runtime decision, then I am all for it. @atmyers takes the lead on this, what do you think?

I just want to reduce the compile-time variations for particle containers at all cost, instead of driving them up further. For instance, the allocator being a compile-time option in particles (vs. a runtime one in MultiFabs - which is better) is another design (as in: maintainability, compile-time, link-time, extensibility vs. performance benefit) issue we should address another time (off-topic) #3643

@AlexanderSinn
Copy link
Member

Using amrex::PolymorphicArenaAllocator the allocator can be changed at runtime. That way only some components could have a different allocator.

@ax3l ax3l changed the title [Draft] Pure SoA Particle: Separate Array for IdCPU Pure SoA Particle: Separate Array for IdCPU Dec 5, 2023
@ax3l ax3l marked this pull request as ready for review December 5, 2023 03:24
@ax3l ax3l force-pushed the topic-pureSoA-separate-64bit-idcpu branch from db64f15 to 105c422 Compare December 8, 2023 17:19
Copy link
Member Author

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you Andrew for your help!

I pushed a minor cleanup and inline comment and added a quick question on the new template argument in StructOfArrays.

Src/Particle/AMReX_ParticleTile.H Outdated Show resolved Hide resolved
@atmyers atmyers enabled auto-merge (squash) December 11, 2023 19:20
@atmyers atmyers merged commit d884f44 into AMReX-Codes:development Dec 11, 2023
69 checks passed
@ax3l ax3l deleted the topic-pureSoA-separate-64bit-idcpu branch December 11, 2023 21:15
ax3l added a commit to ax3l/pyamrex that referenced this pull request Dec 11, 2023
Update to latest commit in `development`.

Introduces the new `idcpu` treatment in pure SoA particles:
AMReX-Codes/amrex#3585
ax3l added a commit to ax3l/pyamrex that referenced this pull request Dec 11, 2023
Update to latest commit in `development`.

Introduces the new `idcpu` treatment in pure SoA particles:
AMReX-Codes/amrex#3585
@ax3l ax3l mentioned this pull request Dec 12, 2023
5 tasks
atmyers pushed a commit that referenced this pull request Dec 12, 2023
## Summary

From #3585 commit: fixes a segfault for the legacy particle layout:
```
Thread 1 "python3" received signal SIGSEGV, Segmentation fault.
0x00007ffff5951e5c in amrex::ParticleTile<amrex::Particle<1, 1>, 2, 1, std::allocator>::push_back<2, 1, 0> (this=0x555555f206e0, sp=...) at /home/axel/src/pyamrex/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleTile.H:916
916	            m_soa_tile.GetIdCPUData()[np] = sp.m_idcpu;
```

## Additional background

X-ref: AMReX-Codes/pyamrex#232

## Checklist

The proposed changes:
- [x] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate
ax3l added a commit to AMReX-Codes/pyamrex that referenced this pull request Dec 12, 2023
* AMReX: Update to latest development

Update to latest commit in `development`.

Introduces the new `idcpu` treatment in pure SoA particles:
AMReX-Codes/amrex#3585

* ImpactX 2024: Pure SoA Update

* AMReX: Update to latest development
guj pushed a commit to guj/amrex that referenced this pull request Dec 13, 2023
## Summary

This addresses a regression we see when moving to pure SoA particles:
- slightly slower read/write to Ids when needed, e.g., for sorting
- issues going up to the full 64bit range

## Additional background

Once finished, this will close AMReX-Codes#3569.

## Checklist

The proposed changes:
- [ ] fix a bug or incorrect behavior in AMReX
- [x] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate

---------

Co-authored-by: Andrew Myers <[email protected]>
guj pushed a commit to guj/amrex that referenced this pull request Dec 13, 2023
## Summary

From AMReX-Codes#3585 commit: fixes a segfault for the legacy particle layout:
```
Thread 1 "python3" received signal SIGSEGV, Segmentation fault.
0x00007ffff5951e5c in amrex::ParticleTile<amrex::Particle<1, 1>, 2, 1, std::allocator>::push_back<2, 1, 0> (this=0x555555f206e0, sp=...) at /home/axel/src/pyamrex/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleTile.H:916
916	            m_soa_tile.GetIdCPUData()[np] = sp.m_idcpu;
```

## Additional background

X-ref: AMReX-Codes/pyamrex#232

## Checklist

The proposed changes:
- [x] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate
@AlexanderSinn AlexanderSinn mentioned this pull request Dec 14, 2023
5 tasks
atmyers pushed a commit that referenced this pull request Dec 15, 2023
## Summary

I noticed a few issues in AMReX while trying to update HiPACE++ for
#3585.

Additionally, I would like to point out that `ParticleTile` has
`push_back_real` and `push_back_int` functions but for PureSoA there is
no `push_back_idcpu`, however this is not added in this PR.

## Additional background

## Checklist

The proposed changes:
- [x] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate
@ax3l ax3l mentioned this pull request Feb 20, 2024
5 tasks
atmyers pushed a commit that referenced this pull request Feb 20, 2024
## Summary

This is a leftover from an earlier implementation of pure SoA. Our 39bit
particle ids must be stored in an `amrex::Long`. An `int` will crop them
to 32bit range, which we quickly run out from.

## Additional background

Related to  #3569 / #3585.

## Checklist

The proposed changes:
- [x] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate
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.

Pure SoA Particles: cpuid as Separate Array
3 participants