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

CArena: Implement alloc_in_place #3426

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

WeiqunZhang
Copy link
Member

First we try to see if CArena has free space that can be used to extend the given pointer to the specified size range. If that's unsuccessful, we use the normal alloc function to allocate memory.

If in-place allocation is successful during vector resizing, we can avoid the cost of copying data to the new allocation address.

@WeiqunZhang WeiqunZhang requested a review from atmyers July 18, 2023 02:07
@WeiqunZhang
Copy link
Member Author

@AlexanderSinn This new PR does not work with the memory profiling in TinyProfiler. Currently, we call TinyProfiler::memory_alloc to register memory allocation and TinyProfiler::memory_free to deregister. The issue now is the MemStat stored in m_busylist is associated with the first memory allocation that created the Node in m_busylist. Now, when we have additional memory alloations that extend a busy list's node, we need to find a way to store those extra MemStats. It's not clear how to do it. Make it a vector (std::vector<MemStat*>)?

@WeiqunZhang
Copy link
Member Author

@AlexanderSinn Maybe we should de-register first and then re-register it. That would be the simplest approach.

First we try to see if CArena has free space that can be used to extend the
given pointer to the specified size range. If that's unsuccessful, we use
the normal alloc function to allocate memory.

If in-place allocation is successful during vector resizing, we can avoid
the cost of copying data to the new allocation address.
@WeiqunZhang WeiqunZhang marked this pull request as ready for review July 18, 2023 05:11
@AlexanderSinn
Copy link
Member

Yes, de-registering first and then re-registering should work fine.

It would be interesting to know from some sort of profiler or printout how often alloc_in_place succeeds.

However, because it is correctly de-register first, while with a normal resize the new allocation would be registered first before the old one is de-registered, TinyProfiler MaxMem should be smaller with this PR if alloc_in_place succeeds for the most expensive resizes.

@AlexanderSinn
Copy link
Member

I tested alloc_in_place using HiPACE++ and it achieved a 36% success rate of all non-trivial cases. However, it only had a small impact on MaxMem of about 0.1% reduction (2 KiB). This is more or less expected since the function that resizes vectors has many small vectors and doesn’t use much memory in total anyway.

@WeiqunZhang WeiqunZhang merged commit d92232a into AMReX-Codes:development Aug 8, 2023
66 checks passed
@WeiqunZhang WeiqunZhang deleted the arena_in_place branch August 8, 2023 16:44
WeiqunZhang added a commit that referenced this pull request Nov 13, 2023
## Summary

Implement CArena::shrink_in_place, which is used by
PODVector::shrink_to_fit. It avoids a new memory allocation and data
movement.

Add operator<< to CArena. This helps debugging.

## Additional background

Follow-up on #3426.

## 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
guj pushed a commit to guj/amrex that referenced this pull request Dec 13, 2023
## Summary

Implement CArena::shrink_in_place, which is used by
PODVector::shrink_to_fit. It avoids a new memory allocation and data
movement.

Add operator<< to CArena. This helps debugging.

## Additional background

Follow-up on AMReX-Codes#3426.

## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants