Repository navigation
[arcane,mesh] Leave a connectivity that never gets a connected item unallocated - #2835
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (13.66%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2835 +/- ##
==========================================
- Coverage 71.45% 71.38% -0.08%
==========================================
Files 2579 2581 +2
Lines 195176 195302 +126
Branches 23172 23199 +27
==========================================
- Hits 139470 139413 -57
- Misses 47057 47223 +166
- Partials 8649 8666 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2fe4e93 to
27792cf
Compare
grospelliergilles
left a comment
grospelliergilles
left a comment
There was a problem hiding this comment.
Thank you for this PR.
It will greatly help reduce memory usage.
There is small things to change. The most important is to not make the new behavior the default until we have validated it.
| // A connectivity that never received a connected item keeps its two per-source | ||
| // variables empty and reads through a shared zero buffer. That is an internal | ||
| // state: a protection must hold the arrays themselves, so that any build can | ||
| // read it back. | ||
| for (const auto& c : m_source_incremental_item_connectivities) | ||
| if (auto* ic = dynamic_cast<IncrementalItemConnectivityBase*>(c.get())) | ||
| ic->materializeForDump(); | ||
|
|
There was a problem hiding this comment.
Is it really needed to materialize memory ?
prepareForDump() is always called from endAllocate() so all the memory usage saved during creation with the memory pool is lost when we call this method.
Also, you can put materializeForDump() in internal API (the interface IIncrementalItemConnectivityInternal) so you do not need the dynamic_cast. You will only do c->_internalApi()->materializeForDump().
| /*! | ||
| * \brief Zeros backing every lazy view of the process. | ||
| * | ||
| * One pool, and not one buffer per connectivity: they would all hold the same | ||
| * thing. A buffer is never resized once published; growing means allocating a | ||
| * new one and keeping the old one alive, because Arcane can build one mesh per | ||
| * subdomain thread and reallocating under another thread's published view is a | ||
| * use after free. What is kept is the geometric series, about twice the | ||
| * largest, and there are only log2(max item count) buffers. | ||
| * | ||
| * A connectivity republishes from its own notify path, when its own localIds | ||
| * outgrow the view it published, so its view always covers its own items | ||
| * without depending on what another thread did. | ||
| */ | ||
| static std::vector<std::unique_ptr<UniqueArray<Int32>>> m_zero_view_pool; | ||
| static std::mutex m_zero_view_mutex; | ||
| //! How many connectivities are still lazy, i.e. still reading the pool | ||
| static Int32 m_nb_lazy; | ||
|
|
There was a problem hiding this comment.
Put all the logic about lazy pool in a separate class. For example you can call it 'LazyMemoryPoolandIncrementalItemConnectivityContainerwill have astaticinstance of this class. So this classLazyMemoryPoolwill have 3 fieldsm_zero_view_pool, m_zero_view_mutexandm_nb_lazy`.
| Integer c = 1024; | ||
| while (c < n) | ||
| c *= 2; | ||
| auto buf = std::make_unique<UniqueArray<Int32>>(); |
There was a problem hiding this comment.
Use MemoryUtils::getDefaultDataAllocator() instead of the default memory allocator for UniqueArray to make its memory available on accelerators.
| auto v = Convert::Type<Int32>::tryParseFromEnvironment("ARCANE_NO_LAZY_CONNECTIVITY", true); | ||
| return v && v.value() != 0; | ||
| }(); | ||
| return is_disabled; |
There was a problem hiding this comment.
You can put this code in the constructor of IncremantalItemConnectivity.
Also reverse the logic and enable lazy connectivity only if an environment variable ARCANE_LAZY_CONNECTIVITY is true. It will allow us to validate these modifications to make sure it works for every cases.
| buf->resize(c); | ||
| buf->fill(0); | ||
| ConstArrayView<Int32> view = buf->constView(); | ||
| m_zero_view_pool.push_back(std::move(buf)); |
There was a problem hiding this comment.
It seems the number of arrays in the memory pool will only grow and the intermediary arrays will only be destroyed when the mesh is destroyed. It is not important to fix it at the moment but this is something to remember
10a1472 to
3aa709b
Compare
|
Thank you for the review. All comments are addressed:
Measured 11% acceleration on mesh build and 33% less memory taken by connectivities with ARCANE_LAZY_CONNECTIVITY=1 |
|
Thank you very much. |
3aa709b to
bb2f48b
Compare
…nallocated 64% of the notifySourceItemAdded of a cartesian build are for connectivities that stay empty for the whole run. While one has never had a connected item its two per-source variables stay at size 0 and its views are served from a shared buffer of zeros; the first addConnectedItem(), or the first write of a protection, allocates for real. The zero buffers grow by powers of two while the mesh is built. At the end of each mesh update every lazy view moves to one buffer of exactly the size of the largest family, and a buffer is freed when its last reader leaves it. 11.1% gain on the mesh build, on a 640k-cell cartesian mesh at one rank. Off by default: ARCANE_LAZY_CONNECTIVITY=1 enables it.
bb2f48b to
be21e59
Compare
|
29ef1c1
into
arcaneframework:main
64% of the notifySourceItemAdded of a cartesian build are for connectivities that stay empty for the whole run. While one has never had a connected item its two per-source variables stay at size 0 and its views are served from a shared buffer of zeros; the first addConnectedItem() allocates for real. Checkpoint and restart are covered: notifyReadFromDump() materialises.
7.0% gain on the mesh build, on a 640k-cell cartesian mesh at one rank.