Skip to content

minimize save/restore allocations #909

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

minimize save/restore allocations #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 71 additions & 33 deletions src/amr/solvers/solver_ppc.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <SAMRAI/hier/Patch.h>

#include "core/vector.hpp"

#include "amr/messengers/hybrid_messenger.hpp"
#include "amr/messengers/hybrid_messenger_info.hpp"
Expand All @@ -18,12 +19,13 @@
#include "core/numerics/faraday/faraday.hpp"
#include "core/numerics/ohm/ohm.hpp"

#include "core/utilities/cellmap.hpp"
#include "core/data/vecfield/vecfield.hpp"
#include "core/data/grid/gridlayout_utils.hpp"


#include <iomanip>
#include <sstream>
#include <tuple>

namespace PHARE::solver
{
Expand Down Expand Up @@ -130,7 +132,7 @@ class SolverPPC : public ISolver<AMR_Types>
double const currentTime, double const newTime, core::UpdaterMode mode);


void saveState_(level_t& level, ModelViews_t& views);
void saveState_(level_t const& level, ModelViews_t const& views);
void restoreState_(level_t& level, ModelViews_t& views);


Expand All @@ -148,26 +150,32 @@ class SolverPPC : public ISolver<AMR_Types>
};


// extend lifespan
std::unordered_map<std::string, ParticleArray> tmpDomain;
std::unordered_map<std::string, ParticleArray> patchGhost;

template<typename Map>
static void add_to(Map& map, std::string const& key, ParticleArray const& ps)
struct SaveState
{
// vector copy drops the capacity (over allocation of the source)
// we want to keep the overallocation somewhat - how much to be assessed
ParticleArray empty{ps.box()};

if (!map.count(key))
map.emplace(key, empty);
else
map.at(key) = empty;

auto& v = map.at(key);
v.reserve(ps.capacity());
v.replace_from(ps);
}
bool constexpr static copy_old = false;
using box_t = core::Box<int, dimension>;
using CellMap_t = core::CellMap<dimension, int>;
using Particle_t = typename ParticleArray::value_type;
using ParticleVec_t = core::MinimizingVector<Particle_t, copy_old>;

using SizeVec_t = core::MinimizingVector<std::size_t, copy_old>;
using CellMapVec_t = core::MinimizingVector<CellMap_t, copy_old>;

auto operator()() { return std::forward_as_tuple(particles(), sizes(), maps()); }
auto operator()(std::size_t const& nparts, std::size_t narrays)
{
if (narrays < sizes.capacity())
narrays += narrays * .01;
return std::forward_as_tuple(particles(nparts), sizes(narrays), maps(narrays));
}

ParticleVec_t particles;
SizeVec_t sizes;
CellMapVec_t maps;
};

// extend lifespan
SaveState domainState, patchGhostState;

}; // end solverPPC

Expand Down Expand Up @@ -214,19 +222,42 @@ void SolverPPC<HybridModel, AMR_Types>::fillMessengerInfo(


template<typename HybridModel, typename AMR_Types>
void SolverPPC<HybridModel, AMR_Types>::saveState_(level_t& level, ModelViews_t& views)
void SolverPPC<HybridModel, AMR_Types>::saveState_(level_t const& level, ModelViews_t const& views)
{
PHARE_LOG_SCOPE(1, "SolverPPC::saveState_");

std::size_t arrays = 0, dcount = 0, pcount = 0;
for (auto& state : views)
{
std::stringstream ss;
ss << state.patch->getGlobalId();
for (auto& pop : state.ions)
{
std::string const key = ss.str() + "_" + pop.name();
add_to(tmpDomain, key, pop.domainParticles());
add_to(patchGhost, key, pop.patchGhostParticles());
++arrays;
dcount += pop.domainParticles().capacity();
pcount += pop.patchGhostParticles().capacity();
}

auto [dParts, dSizes, dMaps] = domainState(dcount, arrays);
auto [pParts, pSizes, pMaps] = patchGhostState(pcount, arrays);

std::size_t arr_idx = 0, doff = 0, poff = 0;
for (auto const& state : views)
{
for (auto const& pop : state.ions)
{
auto& dInParts = pop.domainParticles();
auto& pInParts = pop.patchGhostParticles();

dMaps[arr_idx] = dInParts.map();
pMaps[arr_idx] = pInParts.map();

dSizes[arr_idx] = dInParts.size();
pSizes[arr_idx] = pInParts.size();

std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff);
std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff);
Comment on lines +255 to +256
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure safe copying of particle data with capacity checks

When using std::copy to transfer particle data, it's important to ensure that the destination buffers have sufficient capacity to prevent buffer overflows. Consider adding assertions to verify the capacities before copying.

Apply this diff to add capacity checks:

+                assert((doff + dSizes[arr_idx]) <= dParts.capacity());
+                assert((poff + pSizes[arr_idx]) <= pParts.capacity());

                 std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff);
                 std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff);
std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff);
assert((doff + dSizes[arr_idx]) <= dParts.capacity());
assert((poff + pSizes[arr_idx]) <= pParts.capacity());
std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff);
std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff);


doff += dSizes[arr_idx];
poff += pSizes[arr_idx];
++arr_idx;
}
}
}
Expand All @@ -236,15 +267,22 @@ void SolverPPC<HybridModel, AMR_Types>::restoreState_(level_t& level, ModelViews
{
PHARE_LOG_SCOPE(1, "SolverPPC::restoreState_");

auto const [dInParts, dSizes, dMaps] = domainState();
auto const [pInParts, pSizes, pMaps] = patchGhostState();

std::size_t arr_idx = 0, doff = 0, poff = 0;
for (auto& state : views)
{
std::stringstream ss;
ss << state.patch->getGlobalId();

for (auto& pop : state.ions)
{
pop.domainParticles() = std::move(tmpDomain.at(ss.str() + "_" + pop.name()));
pop.patchGhostParticles() = std::move(patchGhost.at(ss.str() + "_" + pop.name()));
pop.domainParticles().replace_from(dInParts.data() + doff, dSizes[arr_idx],
dMaps[arr_idx]);
pop.patchGhostParticles().replace_from(pInParts.data() + poff, pSizes[arr_idx],
pMaps[arr_idx]);

doff += dSizes[arr_idx];
poff += pSizes[arr_idx];
++arr_idx;
}
}
}
Expand Down
21 changes: 13 additions & 8 deletions src/core/data/particles/particle_array.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,7 @@ class ParticleArray


public:
ParticleArray(box_t box)
: box_{box}
, cellMap_{box_}
{
assert(box_.size() > 0);
}

ParticleArray(box_t box, std::size_t size)
ParticleArray(box_t box = box_t{}, std::size_t size = 0)
: particles_(size)
, box_{box}
, cellMap_{box_}
Expand Down Expand Up @@ -80,6 +73,9 @@ class ParticleArray
return (this->particles_ == that.particles_);
}

NO_DISCARD auto data() const { return particles_.data(); }
NO_DISCARD auto data() { return particles_.data(); }

NO_DISCARD auto begin() const { return particles_.begin(); }
NO_DISCARD auto begin() { return particles_.begin(); }

Expand Down Expand Up @@ -235,6 +231,7 @@ class ParticleArray
NO_DISCARD auto& vector() const { return particles_; }

auto& box() const { return box_; }
auto& map() const { return cellMap_; }


auto& replace_from(ParticleArray const& that)
Expand All @@ -247,6 +244,14 @@ class ParticleArray
this->cellMap_ = that.cellMap_;
return *this;
}
auto& replace_from(Particle_t const* particles, std::size_t size, CellMap_t const& map)
{
this->resize(size);
this->cellMap_ = map;
this->box_ = map.box();
std::copy(particles, particles + size, particles_.data());
return *this;
}


private:
Expand Down
2 changes: 2 additions & 0 deletions src/core/def.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ NO_DISCARD bool isSettable(auto const&... args)
return (check(args) && ...);
}



} // namespace PHARE::core

#endif // PHARE_CORE_DEF_HPP
2 changes: 1 addition & 1 deletion src/core/utilities/cellmap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class CellMap


public:
CellMap(Box<cell_index_t, dim> box)
CellMap(Box<cell_index_t, dim> box = {})
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Potential issues with empty Box usage in CellMap

The recent change allows creating a CellMap instance without providing a Box, which can result in an empty Box. However, several parts of the codebase use box_ without verifying if it is empty:

  • loc[i] -= box_.lower[i]; (line 214)
  • cellIndexes_{box.shape().toArray<std::uint32_t>()} (line 36)

Only one existing check for an empty Box is found at line 228. This inconsiderate handling may lead to undefined behavior or runtime errors when Box is empty.

Recommendations:

  1. Add Null Checks: Ensure all methods accessing box_ verify that it is not empty before usage.
  2. Handle Initialization Safely: Modify the initialization of cellIndexes_ to account for an empty Box.
  3. Comprehensive Testing: Implement tests to cover scenarios where CellMap is initialized with an empty Box.
🔗 Analysis chain

Consider the implications of allowing default construction with an empty Box

The addition of a default argument {} for the box parameter allows creating a CellMap instance without explicitly providing a Box. While this can simplify usage in some cases, it may lead to unintended behavior if the code assumes a non-empty Box.

Consider the following:

  1. Ensure that all methods using box_ (e.g., addToCell, size) can handle an empty Box correctly.
  2. Review the initialization of cellIndexes_ using box.shape() to ensure it behaves correctly with an empty Box.
  3. Add documentation to clarify the behavior and potential limitations of using an empty Box.
  4. Consider adding an is_valid() or similar method to check if the CellMap has been properly initialized with a non-empty Box.

To verify the impact of this change, please run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential issues with empty Box usage

# Test: Search for methods that use box_ member
echo "Methods potentially affected by empty Box:"
rg --type cpp -n 'box_\.' src/core/utilities/cellmap.hpp

# Test: Check cellIndexes_ initialization
echo "\ncellIndexes_ initialization:"
rg --type cpp -n 'cellIndexes_.*box' src/core/utilities/cellmap.hpp

# Test: Search for existing null checks or empty box handling
echo "\nExisting empty box handling:"
rg --type cpp -n 'box_\.isEmpty\(\)|box_\.empty\(\)' src/core/utilities/cellmap.hpp

Length of output: 679

: box_{box}
, cellIndexes_{box.shape().template toArray<std::uint32_t>()}
{
Expand Down
55 changes: 55 additions & 0 deletions src/core/vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#ifndef PHARE_CORE_VECTOR_HPP
#define PHARE_CORE_VECTOR_HPP

#include <vector>
#include <cstdint>
#include "core/utilities/span.hpp"

namespace PHARE::core
{


template<typename T, bool copy_old_ = true>
struct MinimizingVector
{
template<bool copy_old = copy_old_>
auto& get(std::size_t const& s)
{
if (s < v.capacity() * percentile)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential precision loss due to implicit conversion in comparison

In line 18:

if (s < v.capacity() * percentile)

The expression v.capacity() * percentile results in a double, while s is a std::size_t (an unsigned integer). Comparing an integer to a floating-point number can lead to unexpected behavior due to implicit conversion and precision loss.

Consider explicitly casting the result to std::size_t to ensure correct comparison:

if (s < static_cast<std::size_t>(v.capacity() * percentile))

This ensures that both sides of the comparison are of the same type, avoiding potential precision issues.

++_c;
else
_c = 0;

if (_c == period)
{
std::vector<T> r(v.capacity() * realloc_to);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implicit conversion from double to size_t may cause truncation

In line 25:

std::vector<T> r(v.capacity() * realloc_to);

The expression v.capacity() * realloc_to produces a double, but the std::vector constructor expects a size_t for its size parameter. Implicitly converting a double to a size_t can truncate the value, possibly leading to smaller allocations than intended.

Apply this fix to ensure the size is correctly calculated:

- std::vector<T> r(v.capacity() * realloc_to);
+ std::vector<T> r(static_cast<std::size_t>(v.capacity() * realloc_to));

Alternatively, you can use rounding functions if partial capacities should influence the size:

std::vector<T> r(static_cast<std::size_t>(std::ceil(v.capacity() * realloc_to)));

if constexpr (copy_old)
r = v;
v = std::move(r);
_c = 0;
}

v.resize(s);
return v;
}

auto size() const { return v.size(); }
auto capacity() const { return v.capacity(); }
auto& operator()() { return v; }
auto& operator()() const { return v; }
auto& operator()(std::size_t const& s) { return get(s); }
Comment on lines +38 to +40
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Overloading operator() may reduce code readability

Overloading the function call operator to access or modify the internal vector can be confusing, as it's unconventional for object manipulation.

Lines 38-40:

auto& operator()() { return v; }
auto& operator()() const { return v; }
auto& operator()(std::size_t const& s) { return get(s); }

Consider using explicit member function names for clarity:

  • Replace operator()() with data() to access the vector.
  • Replace operator()(std::size_t const& s) with resize(s) or retain get(s).

Refactored example:

auto& data() { return v; }
auto& data() const { return v; }
auto& resize(std::size_t const& s) { return get(s); }

This makes the interface more intuitive and improves code expressiveness.


double const percentile = .80;
double const realloc_to = .90;
std::size_t const period = 100;

std::vector<T> v{};
std::uint16_t _c = 0;
};




} // namespace PHARE::core

#endif // PHARE_CORE_VECTOR_HPP
Loading