Optimise Memory Consumption during Setup #2005
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…umonguous-buffer-of-all-the-labels-in-the-world
…umonguous-buffer-of-all-the-labels-in-the-world
…umonguous-buffer-of-all-the-labels-in-the-world
AdhocMan
requested changes
Jan 26, 2023
…umonguous-buffer-of-all-the-labels-in-the-world
AdhocMan
previously approved these changes
Jan 27, 2023
|
do we still need the struct |
|
No and it should no longer be here. |
boeschf
reviewed
Mar 7, 2023
Comment on lines
+369
to
+370
| res.max_rss = (n.max_rss == -1) ? "-" : std::to_string(n.max_rss); | ||
| res.fst_rss = (n.fst_rss == -1) ? "-" : std::to_string(n.fst_rss); |
There was a problem hiding this comment.
out of curiosity: why are you not using snprintf here?
There was a problem hiding this comment.
No particular reason, that was just the way it was done here before.
arbor/communication/communicator.cpp
Outdated
Comment on lines
148
to
149
| if (!sources.count(sgid)) sources.emplace(sgid, label_map{sgid, rec}); | ||
| auto src_lid = sources.at(sgid).resolve(c.source); |
There was a problem hiding this comment.
Suggested change
| if (!sources.count(sgid)) sources.emplace(sgid, label_map{sgid, rec}); | |
| auto src_lid = sources.at(sgid).resolve(c.source); | |
| auto src_lid = sources.try_emplace(sgid, sgid, rec).first->second.resolve(c.source); |
There was a problem hiding this comment.
So, if I do this, will the value be constructed? If so, it's precisely running against what I want.
… on test 8.13MB -> 4.63MB
thorstenhater
changed the title
Ponder whether we can make MT build more efficient.
|
Looks like this'll be essential for large simulations, the use-case we promote Arbor for. Is it possible to provide either strategy as a user option? Give me fast init, or give me low memory usage? Or even auto-activate the low mem path upon ncells > threshold (e.g. 100M?). If not that, maybe a troubleshooting checklist for when e.g. people run out of memory or other fail situations. |
This was referenced Aug 11, 2023
2 tasks
thorstenhater
added a commit
that referenced
this pull request
Nov 29, 2023
Use the simple but well-known FNV-1a hash function to map `cell_tag_type` aka `std::string` to an `uint64_t` for label resolution. The former type has a size of 32B or more and the latter 8B, thus cutting the storage and bandwidth requirements by 3/4. The hash function is implemented from the reference given on the authors' website/wikipedia and is extremely simple. If we ever experience issues, we might consider switching this to something of higher quality via an external library, candidates are `xxHASH` and `Murmur3`. https://github.com/Cyan4973/xxHash Note: This should further relieve the memory pressure on larger scale simulation as formulated in #1969 and make #2005 less urgent. There is no performance impact (at laptop scale), but the memory savings are worth it.
thorstenhater
added a commit
that referenced
this pull request
Feb 27, 2024
Spin-off from #2005. Make the primary load balancing cleaner and faster and more maintainable. Thus: - remove all MPI calls, this is now purely local - remove temporary data structures and/or coral them into their own little scopes - simplify `super_cells` vs `regular_cells` - sort less - sparse connection tables Partially inspired by external feedback
|
This has been splintered and merged in those splinters. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction
Historically, our setup phase is quite cavalier with performance and memory which has given rise
to various concerns for larger networks. As we are preparing for large deployments in the next
HPC generation, we need to address this.
Tests on MPI are still failing for domain decomposition due to a cyclic shift of groups
which shouldn't influence the test or functionality. But I am too stupid to fix the test to
pass.
Label Resolution
Our label resolution scheme concatenates a global vector of all source labels across all MPI tasks.$\mathcal{O}(10)$ --- labels per cell.
In my recent experiments, this exhausts the node-local memory (512GB) at around 150M cells,
with only a few ---
Fix
In each cell group, build a lazy map of the GIDs we are connected to and the associated source
labels. This reduces the required memory to the actual high-water-mark, but incurs the cost
of instantiating cells multiple times.
If this still not enough, we can proceed to minimize the memory consumption even more by
(simple) evict labels from the cache if we are close to the limit. Costs more instantiations, butDonesaves memory
long as needed. Now memory is O(1), but we need to fiddle with construction order all the while
keeping the connectivity stable. Which might be complicated
Thingification during Cable Cell Construction
Cable cells constantly invoke
thingify(region, provider)during construction. Mostly however, wepaintthose region in succession, ie:This costs a lost of temporary allocations.
Fix
Simply cache the last region, following the idea that this is the 90% case.
Temporary data structures during Setup
embed_pwlinis particularly bad with managing allocation so much so that it shows up on timeprofiles as a major cost centre. This originates from structures which are nested vectors under the
hood and must return values for reasons of genericity.
Fix
Return references where possible, convert the rest to continuation passing style so we never actually
return a value but take a callback which is invoked with the references.
mechanism_catalogueallocates temporariesoperator[]returns a value, where a const reference would be sufficient.Fix
Obvious, no?
General
Split large functions -- especially in
fvm_layout-- into smaller ones. This fences in temporary memory allocations.Clean-up and Extras
Rename
mc_cell_*to the propercable_cell_*This also renames -- finally! --
mc_cell_*tocable_cell_*Add RSS field to Profiler
To check for the actual memory consumption recording of the high watermark memory was added to the
profiler. It now prints the memory used at the 1st hit of this cost centre and the associated maximum. Thus,
we can determine allocation during runtime and setup.
Example (not very helpful, but illustrative)
Add logging
operator newThis will -- if enabled at compile time -- spit out the amount of Bytes allocated and the callstack⚠️ This is hilariously slow, but really helps pinpointing
of the allocation site to
stderrduring runtime.allocations. Use the
script/memory-log-to-profile.pyto convert log files to a report like this:Linked
Fixes #1969