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.

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

Jump to bottom

[Core][PVS] Add PointerVectorSet::MutablePass to allow push_back safely. #12839

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
+172 −100
Open

[Core][PVS] Add PointerVectorSet::MutablePass to allow push_back safely. #12839

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ab23b59
fix operators
sunethwarna Nov 8, 2024
32d9d48
add static assert
sunethwarna Nov 8, 2024
0fb1461
revert changes
sunethwarna Nov 8, 2024
24e7167
add MutablePass
sunethwarna Nov 9, 2024
8d6e0a2
Merge branch 'core/pvs/fix_operators' into core/pvs/add_accessor
sunethwarna Nov 9, 2024
6510609
minor error message change
sunethwarna Nov 9, 2024
1ee6ca2
adding some additional methods
sunethwarna Nov 11, 2024
0b3d72a
minor
sunethwarna Nov 11, 2024
78ac0ff
fix hdf5 application
sunethwarna Nov 11, 2024
ed74f0d
minor
sunethwarna Nov 11, 2024
d2ce128
remove std::atomic
sunethwarna Nov 11, 2024
c03c7b8
include parallel_utils header
sunethwarna Nov 11, 2024
0ca518b
fix combine_model_part_modeler
sunethwarna Nov 12, 2024
2aee55f
fix further cases which can break PVS
sunethwarna Nov 21, 2024
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
6 changes: 4 additions & 2 deletions applications/HDF5Application/custom_io/hdf5_connectivities_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ void ConnectivitiesData::CreateEntities(NodesContainerType& rNodes,
KRATOS_ERROR_IF(geometry_size != mConnectivities.size2())
<< "Non-matching geometry and connectivity sizes." << std::endl;
rElements.reserve(rElements.size() + num_new_elems);
auto mutable_pass = rElements.GetMutablePass();
ElementType::NodesArrayType nodes(geometry_size);

for (unsigned i = 0; i < num_new_elems; ++i)
Expand All @@ -90,7 +91,7 @@ void ConnectivitiesData::CreateEntities(NodesContainerType& rNodes,
}
ElementType::Pointer p_elem =
r_elem.Create(mIds[i], nodes, rProperties(mPropertiesIds[i]));
rElements.push_back(p_elem);
mutable_pass.push_back(p_elem);
Copy link
Member

Choose a reason for hiding this comment

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

I would change this (and the others) for:

ElementType::NodesArrayType nodes(geometry_size);

{
    auto mutable_pass = rElements.GetMutablePass();
    for (unsigned i = 0; i < num_new_elems; ++i) {
        for (unsigned j = 0; j < geometry_size; ++j) {
            const int node_id = mConnectivities(i, j);
            nodes(j) = rNodes(node_id);
        } 
        ElementType::Pointer p_elem = r_elem.Create(mIds[i], nodes, rProperties(mPropertiesIds[i]));
        mutable_pass.push_back(p_elem);
    }
}

While I like the idea of the MutablePass imho, it has a very big problem: You cannot control its life cycle and its correct usage its tied to it. I understand that this option is preferred over a Lock/Unlock mechanism over all the PointerVectorSet but if this is the case we should be careful while utilizing it and restrict it only to the scope in which is going to be valid.

While is not a problem in the usages that were modified in this PR because in very well encapsulated locations, I think it will become a concern later on, and at least this way we are hinting people that just copy/paste the code that they need to be aware of this detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only way of ensuring correct usage would be getting rid of stuff that break the container's contract (such as push_back) altogeher.

This is unfortunately impossible because the KTC insists on having such features, so the next best thing @sunethwarna and I could come up with is this (= taking away as much control from the user as possible). Of course MutablePass can easily be misused (e.g.: heap-allocated) but I don't see anything we could do about that. We're more than open to suggestions though, just not ones that make it easier for users to break uniqueness/sortedness.

Copy link
Member

Choose a reason for hiding this comment

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

Yep I understand, my only comment is, at least lets try to restrict the scope of the mutable to the minimum necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

@roigcarlo I agree, if there are ways to restrict the usage, we would be fully open. :)
@matekelemen: In the case heap allocation, if they keep the mutable pass alive, then all the functions which depends on the sortedness/uniqueness will be disabled, there is a high chance that the simulation will fail (not always for sure).

Copy link
Member Author

@sunethwarna sunethwarna Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One thing we can do to restrict full use when an MutablePass is issued is, to implement begin and end interators within mutable pass, and disable the iterators in the PVS. This adds a cost to normal PVS loops as well which I don't like, but it will for sure make the simulation fail, because i think 99% of the time we need PVS iterators.

Copy link
Member Author

Choose a reason for hiding this comment

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

But my suggestion is to tell people how to use mutable pass, just as an std::lock_guard.

Copy link
Member

Choose a reason for hiding this comment

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

using bracers as in the example should be enough, no need for complex solutions, that all was saying

}
KRATOS_CATCH("");
}
Expand All @@ -108,6 +109,7 @@ void ConnectivitiesData::CreateEntities(NodesContainerType& rNodes,
KRATOS_ERROR_IF(geometry_size != mConnectivities.size2())
<< "Non-matching geometry and connectivity sizes." << std::endl;
rConditions.reserve(rConditions.size() + num_new_conds);
auto mutable_pass = rConditions.GetMutablePass();
ConditionType::NodesArrayType nodes(geometry_size);

for (unsigned i = 0; i < num_new_conds; ++i)
Expand All @@ -119,7 +121,7 @@ void ConnectivitiesData::CreateEntities(NodesContainerType& rNodes,
}
Condition::Pointer p_cond =
r_cond.Create(mIds[i], nodes, rProperties(mPropertiesIds[i]));
rConditions.push_back(p_cond);
mutable_pass.push_back(p_cond);
}
KRATOS_CATCH("");
}
Expand Down
3 changes: 2 additions & 1 deletion applications/HDF5Application/custom_io/hdf5_points_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ void PointsData::CreateNodes(NodesContainerType& rNodes)
KRATOS_TRY;
const unsigned num_new_nodes = mIds.size();
rNodes.reserve(rNodes.size() + num_new_nodes);
auto mutable_pass = rNodes.GetMutablePass();
for (unsigned i = 0; i < num_new_nodes; ++i)
{
const array_1d<double, 3>& r_coord = mCoords[i];
NodeType::Pointer p_node = Kratos::make_intrusive<NodeType>(
mIds[i], r_coord[0], r_coord[1], r_coord[2]);
rNodes.push_back(p_node);
mutable_pass.push_back(p_node);
}
KRATOS_CATCH("");
}
Expand Down
4 changes: 0 additions & 4 deletions applications/HDF5Application/tests/test_hdf5_model_part_io_mpi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,6 @@ def test_HDF5ModelPartIO(self):
self.assertEqual(read_node.Z, write_node.Z)
# Check elements
self.assertEqual(read_model_part.NumberOfElements(), write_model_part.NumberOfElements())
first_elem_id = next(iter(read_model_part.Elements)).Id
read_model_part.GetElement(first_elem_id) # Force a sort since order is mixed by openmp.
for read_elem, write_elem in zip(read_model_part.Elements, write_model_part.Elements):
self.assertEqual(read_elem.Id, write_elem.Id)
self.assertEqual(read_elem.Properties.Id, write_elem.Properties.Id)
Expand All @@ -213,8 +211,6 @@ def test_HDF5ModelPartIO(self):
self.assertEqual(read_elem_node.Id, write_elem_node.Id)
# Check conditions
self.assertEqual(read_model_part.NumberOfConditions(), write_model_part.NumberOfConditions())
first_cond_id = next(iter(read_model_part.Conditions)).Id
read_model_part.GetCondition(first_cond_id) # Force a sort since order is mixed by openmp.
for read_cond, write_cond in zip(read_model_part.Conditions, write_model_part.Conditions):
self.assertEqual(read_cond.Id, write_cond.Id)
self.assertEqual(read_cond.Properties.Id, write_cond.Properties.Id)
Expand Down
208 changes: 144 additions & 64 deletions kratos/containers/pointer_vector_set.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <sstream>
#include <cstddef>
#include <utility>
#include <type_traits>
#include <type_traits>
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated


// External includes
#include <boost/iterator/indirect_iterator.hpp>
Expand All @@ -28,6 +30,7 @@
#include "includes/define.h"
#include "includes/serializer.h"
#include "containers/key_generator.h"
#include "utilities/parallel_utilities.h"

namespace Kratos
{
Expand Down Expand Up @@ -107,6 +110,89 @@ class PointerVectorSet final
using ptr_const_reverse_iterator = typename TContainerType::const_reverse_iterator;
using difference_type = typename TContainerType::difference_type;

///@}
///@name Class definitions
///@{

/// @brief Unrestricted accessor for the pointer vector set
/// @details This class provides unrestricted access the underlying std::vector of the pointer vector
/// set which may make the pointer vector set unsorted. Hence, once an object of this is created, the
/// methods which utilize the sorted property of pointer vector set are frozen. At the destruction of this class,
/// pointer vector set is sorted and made unique, and the frozen methods are released.
/// @param
/// @return
class MutablePass
{
///@name Life cycle
///@{

// Constructor
MutablePass(PointerVectorSet& rContainer)
: mrContainer(rContainer)
{
KRATOS_CRITICAL_SECTION
KRATOS_ERROR_IF(mrContainer.mHasMutablePass)
<< "A mutable pass is active. Hence, creation of an another MutablePass is prohibited.";
mrContainer.mHasMutablePass = true;
}

///@}

public:
///@name Life cycle
///@{

MutablePass(const MutablePass& rOther) = delete;

MutablePass(MutablePass&& rOther) noexcept = default;

// Destructor
~MutablePass()
{
// Sort the PointerVectorSet data
std::sort(mrContainer.mData.begin(), mrContainer.mData.end(), CompareKey());

// Make the entities unique
auto new_end_it = std::unique(mrContainer.mData.begin(), mrContainer.mData.end(), EqualKeyTo());

// remove the duplicated entities.
mrContainer.mData.erase(new_end_it, mrContainer.mData.end());

// unfreeze the methods which dependent on the sorted state of the PointerVectorSet
mrContainer.mHasMutablePass = false;
}

///@name Public operators
///@{

MutablePass& operator=(const MutablePass& rOther) = delete;

///@}
///@name Public methods
///@{

void push_back(TPointerType pValue)
{
mrContainer.mData.push_back(pValue);
}

///@}

private:
///@name Member variables
///@{

PointerVectorSet& mrContainer;

///@}
///@name Friend classes
///@{

friend class PointerVectorSet;

///@}
};

///@}
///@name Life Cycle
///@{
Expand Down Expand Up @@ -178,29 +264,15 @@ class PointerVectorSet final
*/
TDataType& operator[](const key_type& Key)
{
ptr_iterator sorted_part_end;

if (mData.size() - mSortedPartSize >= mMaxBufferSize) {
Sort();
sorted_part_end = mData.end();
KRATOS_ERROR_IF(mHasMutablePass)
Copy link
Member

Choose a reason for hiding this comment

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

No go...you are adding an overhead to every access...

<< "A mutable pass is active. Hence, use of PointerVectorSet::operator[] is prohibited.";
ptr_iterator i(std::lower_bound(mData.begin(), mData.end(), Key, CompareKey()));
if (i != mData.end() && EqualKeyTo(Key)(*i)) {
return **i;
} else {
sorted_part_end = mData.begin() + mSortedPartSize;
}

ptr_iterator i(std::lower_bound(mData.begin(), sorted_part_end, Key, CompareKey()));
if (i == sorted_part_end) {
mSortedPartSize++;
return **mData.insert(sorted_part_end, TPointerType(new TDataType(Key)));
static_assert(!std::is_pointer_v<TPointerType>, "Raw pointers are not supported.");
return **mData.insert(i, TPointerType(new TDataType(Key)));
}

if (!EqualKeyTo(Key)(*i)) {
if ((i = std::find_if(sorted_part_end, mData.end(), EqualKeyTo(Key))) == mData.end()) {
mData.push_back(TPointerType(new TDataType(Key)));
return **(mData.end() - 1);
}
}

return **i;
}

/**
Expand All @@ -213,27 +285,15 @@ class PointerVectorSet final
*/
pointer& operator()(const key_type& Key)
{
ptr_iterator sorted_part_end;

if (mData.size() - mSortedPartSize >= mMaxBufferSize) {
Sort();
sorted_part_end = mData.end();
} else
sorted_part_end = mData.begin() + mSortedPartSize;

ptr_iterator i(std::lower_bound(mData.begin(), sorted_part_end, Key, CompareKey()));
if (i == sorted_part_end) {
mSortedPartSize++;
return *mData.insert(sorted_part_end, TPointerType(new TDataType(Key)));
KRATOS_ERROR_IF(mHasMutablePass)
<< "A mutable pass is active. Hence, use of PointerVectorSet::operator() is prohibited.";
ptr_iterator i(std::lower_bound(mData.begin(), mData.end(), Key, CompareKey()));
if (i != mData.end() && EqualKeyTo(Key)(*i)) {
return *i;
} else {
static_assert(!std::is_pointer_v<TPointerType>, "Raw pointers are not supported.");
return *mData.insert(i, TPointerType(new TDataType(Key)));
}

if (!EqualKeyTo(Key)(*i))
if ((i = std::find_if(sorted_part_end, mData.end(), EqualKeyTo(Key))) == mData.end()) {
mData.push_back(TPointerType(new TDataType(Key)));
return *(mData.end() - 1);
}

return *i;
}

/**
Expand Down Expand Up @@ -568,6 +628,8 @@ class PointerVectorSet final
*/
iterator insert(const TPointerType& value)
{
KRATOS_ERROR_IF(mHasMutablePass)
<< "A mutable pass is active. Hence, use of PointerVectorSet::insert(value) is prohibited.";
auto itr_pos = std::lower_bound(mData.begin(), mData.end(), KeyOf(*value), CompareKey());
if (itr_pos == mData.end()) {
// the position to insert is at the end.
Expand Down Expand Up @@ -596,6 +658,8 @@ class PointerVectorSet final
*/
iterator insert(const_iterator position_hint, const TPointerType& value)
{
KRATOS_ERROR_IF(mHasMutablePass)
<< "A mutable pass is active. Hence, use of PointerVectorSet::insert(position_hint, value) is prohibited.";
if (empty()) {
// the dataset is empty. So use push back.
mData.push_back(value);
Expand Down Expand Up @@ -647,6 +711,8 @@ class PointerVectorSet final
template <class InputIterator>
void insert(InputIterator first, InputIterator last)
{
KRATOS_ERROR_IF(mHasMutablePass)
<< "A mutable pass is active. Hence, use of PointerVectorSet::insert(first, last) is prohibited.";
// first sorts the input iterators and make the input unique.
std::sort(first, last, CompareKey());
auto new_last = std::unique(first, last, EqualKeyTo());
Expand Down Expand Up @@ -741,20 +807,14 @@ class PointerVectorSet final
*/
iterator find(const key_type& Key)
{
ptr_iterator sorted_part_end;

if (mData.size() - mSortedPartSize >= mMaxBufferSize) {
Sort();
sorted_part_end = mData.end();
} else
sorted_part_end = mData.begin() + mSortedPartSize;

ptr_iterator i(std::lower_bound(mData.begin(), sorted_part_end, Key, CompareKey()));
if (i == sorted_part_end || (!EqualKeyTo(Key)(*i)))
if ((i = std::find_if(sorted_part_end, mData.end(), EqualKeyTo(Key))) == mData.end())
return mData.end();

return i;
KRATOS_ERROR_IF(mHasMutablePass)
<< "A mutable pass is active. Hence, use of PointerVectorSet::find(key) is prohibited.";
ptr_iterator i(std::lower_bound(mData.begin(), mData.end(), Key, CompareKey()));
if (i != mData.end() && EqualKeyTo(Key)(*i)) {
return i;
} else {
return mData.end();
}
}

/**
Expand All @@ -767,14 +827,14 @@ class PointerVectorSet final
*/
const_iterator find(const key_type& Key) const
{
ptr_const_iterator sorted_part_end(mData.begin() + mSortedPartSize);

ptr_const_iterator i(std::lower_bound(mData.begin(), sorted_part_end, Key, CompareKey()));
if (i == sorted_part_end || (!EqualKeyTo(Key)(*i)))
if ((i = std::find_if(sorted_part_end, mData.end(), EqualKeyTo(Key))) == mData.end())
return mData.end();

return const_iterator(i);
KRATOS_ERROR_IF(mHasMutablePass)
<< "A mutable pass is active. Hence, use of PointerVectorSet::find(key) is prohibited.";
ptr_const_iterator i(std::lower_bound(mData.begin(), mData.end(), Key, CompareKey()));
if (i != mData.end() && EqualKeyTo(Key)(*i)) {
return const_iterator(i);
} else {
return mData.end();
}
}

/**
Expand Down Expand Up @@ -838,6 +898,11 @@ class PointerVectorSet final
///@name Access
///@{

MutablePass GetMutablePass()
{
return MutablePass(*this);
}

/** Gives a reference to underly normal container. */
TContainerType& GetContainer()
{
Expand Down Expand Up @@ -1062,6 +1127,8 @@ class PointerVectorSet final
/// The maximum buffer size for data storage.
size_type mMaxBufferSize;

bool mHasMutablePass = false;

///@}
///@name Private Operators
///@{
Expand Down Expand Up @@ -1213,7 +1280,7 @@ class PointerVectorSet final
}

///@}
///@name Serialization
///@name Friend classes
///@{

/**
Expand All @@ -1222,6 +1289,19 @@ class PointerVectorSet final
*/
friend class Serializer;

/**
* @class MutablePass
* @brief A friend class responsible for giving a pass to mutate which may destroy the sorted state
* of PointerVectorSet. All methods which relies on the sorted state will be frozen when this is
* active, and they will be unfrozen and PointerVectorSet will be sorted at the destruction on the
* MutablePass.
*/
friend class MutablePass;

///@}
///@name Serialization
///@{

/**
* @brief Extract the object's state and uses the Serializer to store it.
* @param rSerializer Serializer instance to be used for saving.
Expand Down
Loading
Loading