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

[Core] Remove push_back from ModelPart #12903

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
dd02fc8
add additional insert and typecheck to PVS
sunethwarna Dec 5, 2024
2bf1e96
remove push_backs from ModelPart
sunethwarna Dec 5, 2024
2c5d23f
include reduction_utils
sunethwarna Dec 5, 2024
d5521ba
minor
sunethwarna Dec 5, 2024
2f72e0c
func name change
sunethwarna Dec 5, 2024
1e98ea3
fix embedded_skin_utility for Id Change
sunethwarna Dec 10, 2024
9c05b31
allow insertion from const vectors
sunethwarna Dec 10, 2024
abbce15
allow insertion from parent entity ranges
sunethwarna Dec 10, 2024
71ae35f
fix for iterator invalidation
sunethwarna Dec 10, 2024
6d34602
add comments
sunethwarna Dec 10, 2024
f1c4a6f
minor signature modification
sunethwarna Dec 10, 2024
35c1874
fix assign_ms_const_to_neigh_utils
sunethwarna Dec 11, 2024
13e87e7
fix error msg tests
sunethwarna Dec 11, 2024
a50fd2b
fix hdf5
sunethwarna Dec 11, 2024
8a42759
generalize addition from ids
sunethwarna Dec 11, 2024
93293e0
simplify
sunethwarna Dec 11, 2024
6adabf4
fix ranged id addition
sunethwarna Dec 11, 2024
9082f96
Merge remote-tracking branch 'origin/master' into pvs/remove_push_bac…
sunethwarna Dec 12, 2024
d7d4734
fix parmmg
sunethwarna Dec 16, 2024
d718a9a
add benchmarks for modelpart
sunethwarna Dec 16, 2024
4430f5e
minor for comparison
sunethwarna Dec 16, 2024
d7e05de
add type_traits
sunethwarna Dec 18, 2024
fedc6b3
allow rvalue containers in pvs insert
sunethwarna Dec 18, 2024
d642295
allow rvalue containers in model part
sunethwarna Dec 18, 2024
ee29e90
tests for rvalue containers in PVS
sunethwarna Dec 18, 2024
71e3074
more performance improvements
sunethwarna Dec 18, 2024
1024156
fix tests
sunethwarna Dec 18, 2024
a9e8219
minor
sunethwarna Dec 18, 2024
abfdcab
bug fix
sunethwarna Dec 18, 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
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ void ConnectivitiesData::CreateEntities(NodesContainerType& rNodes,
}
ElementType::Pointer p_elem =
r_elem.Create(mIds[i], nodes, rProperties(mPropertiesIds[i]));
rElements.push_back(p_elem);
// here we can safely use the insert with the rElements.end() as the hint
// because, when reading, we can assume that it was written in the
// sorted order within HDF5.
rElements.insert(rElements.end(), 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.

this is not efficient as you may be creating it out of order

Copy link
Member Author

Choose a reason for hiding this comment

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

It cannot be out of order, because we assume HDF5 files are written by the HDF5Application, hence when we write, we always write in the sorted order.

Copy link
Member

Choose a reason for hiding this comment

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

then i do not understand the role of mIds ...

Copy link
Member

Choose a reason for hiding this comment

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

wait, mIds is ordered?

Copy link
Member

Choose a reason for hiding this comment

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

i'll accept the comment, but ... what if somone writes the hdf5 "by hand"? in that case it will be enormously expensive, correct?

}
KRATOS_CATCH("");
}
Expand Down Expand Up @@ -119,7 +122,10 @@ void ConnectivitiesData::CreateEntities(NodesContainerType& rNodes,
}
Condition::Pointer p_cond =
r_cond.Create(mIds[i], nodes, rProperties(mPropertiesIds[i]));
rConditions.push_back(p_cond);
// here we can safely use the insert with the rConditions.end() as the hint
// because, when reading, we can assume that it was written in the
// sorted order within HDF5.
rConditions.insert(rConditions.end(), p_cond);
RiccardoRossi marked this conversation as resolved.
Show resolved Hide resolved
}
KRATOS_CATCH("");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ void PointsData::CreateNodes(NodesContainerType& rNodes)
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);
// here we can safely use the insert with the rNodes.end() as the hint
// because, when reading, we can assume that it was written in the
// sorted order within HDF5.
rNodes.insert(rNodes.end(), p_node);
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment as before

}
KRATOS_CATCH("");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1184,9 +1184,9 @@ void ParMmgUtilities<TPMMGLibrary>::WriteMeshDataToModelPart(
std::unordered_map<IndexType, IndexVectorType> color_nodes, first_color_cond, second_color_cond, first_color_elem, second_color_elem;

// The tempotal store of
ConditionsArrayType created_conditions_vector;
ElementsArrayType created_elements_vector;
NodesArrayType created_nodes_vector;
std::vector<Condition::Pointer> created_conditions_vector;
std::vector<Element::Pointer> created_elements_vector;
std::vector<Node::Pointer> created_nodes_vector;

// Auxiliar values
int ref, is_required;
Expand Down
274 changes: 274 additions & 0 deletions kratos/benchmarks/model_part_benchmark.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,274 @@
// | / |
// ' / __| _` | __| _ \ __|
// . \ | ( | | ( |\__ `
// _|\_\_| \__,_|\__|\___/ ____/
// Multi-Physics
//
// License: BSD License
// Kratos default license: kratos/license.txt
//
// Main authors: Suneth Warnakulasuriya
//

// System includes
#include <vector>

// External includes
#include <benchmark/benchmark.h>

// Project includes
#include "containers/model.h"
#include "containers/pointer_vector_set.h"
#include "includes/indexed_object.h"
#include "includes/key_hash.h"
#include "includes/model_part.h"
#include "includes/smart_pointers.h"
#include "utilities/assign_unique_model_part_collection_tag_utility.h"

namespace Kratos {

static void BM_ModelPartCreateNewNode(benchmark::State& state) {
for (auto _ : state) {
const IndexType input_size = state.range(0);

state.PauseTiming();

auto model = Model();
auto& r_model_part = model.CreateModelPart("test");

state.ResumeTiming();

for (IndexType i = 0; i < input_size; ++i) {
r_model_part.CreateNewNode(i + 1, 0.0, 0.0, 0.0);
}
}
}


static void BM_ModelPartAddNodesToRootFromRange1(benchmark::State& state) {
for (auto _ : state) {
const IndexType input_size = state.range(0);
const bool fill_existing = state.range(1) == 1;

state.PauseTiming();

auto model = Model();
auto& r_model_part = model.CreateModelPart("test");

std::vector<ModelPart::NodeType::Pointer> nodes_to_be_added, existing_nodes;
if (!fill_existing) {
nodes_to_be_added.reserve(input_size);
} else {
nodes_to_be_added.reserve(input_size / 2 + 1);
existing_nodes.reserve(input_size / 2 + 1);
}

// doing it in reverse to make sure a proper sort is done always
for (IndexType i = input_size; i > 0; --i) {
auto p_node = Kratos::make_intrusive<NodeType>(i, 0.0, 0.0, 0.0);
if (!fill_existing || i % 2 == 0) {
nodes_to_be_added.push_back(p_node);
} else {
existing_nodes.push_back(p_node);
}
}
r_model_part.AddNodes(existing_nodes.begin(), existing_nodes.end());
state.ResumeTiming();

// benchmarking to add nodes for already existing model part with nodes
r_model_part.AddNodes(nodes_to_be_added.begin(), nodes_to_be_added.end());
}
}

static void BM_ModelPartAddNodesToRootFromRange2(benchmark::State& state) {
for (auto _ : state) {
const IndexType input_size = state.range(0);
const bool fill_existing = state.range(1) == 1;

state.PauseTiming();

auto model = Model();
auto& r_model_part = model.CreateModelPart("test");

PointerVectorSet<ModelPart::NodeType, IndexedObject> nodes_to_be_added, existing_nodes;
if (!fill_existing) {
nodes_to_be_added.reserve(input_size);
} else {
nodes_to_be_added.reserve(input_size / 2 + 1);
existing_nodes.reserve(input_size / 2 + 1);
}

// doing it in reverse to make sure a proper sort is done always
for (IndexType i = 0; i < input_size; ++i) {
auto p_node = Kratos::make_intrusive<NodeType>(i + 1, 0.0, 0.0, 0.0);
if (!fill_existing || i % 2 == 0) {
nodes_to_be_added.insert(nodes_to_be_added.end(), p_node);
} else {
existing_nodes.insert(existing_nodes.end(), p_node);
}
}
r_model_part.AddNodes(existing_nodes.begin(), existing_nodes.end());
state.ResumeTiming();

// benchmarking to add nodes for already existing model part with nodes
r_model_part.AddNodes(nodes_to_be_added.begin(), nodes_to_be_added.end());
}
}

static void BM_ModelPartAddNodesToSubSubFromRange1(benchmark::State& state) {
for (auto _ : state) {
const IndexType input_size = state.range(0);
const bool fill_existing = state.range(1) == 1;

state.PauseTiming();

auto model = Model();
auto& r_sub_sub_model_part = model.CreateModelPart("test").CreateSubModelPart("sub_test").CreateSubModelPart("sub_sub_test");

std::vector<ModelPart::NodeType::Pointer> nodes_to_be_added, existing_nodes;
if (!fill_existing) {
nodes_to_be_added.reserve(input_size);
} else {
nodes_to_be_added.reserve(input_size / 2 + 1);
existing_nodes.reserve(input_size / 2 + 1);
}

// doing it in reverse to make sure a proper sort is done always
for (IndexType i = 0; i < input_size; ++i) {
auto p_node = Kratos::make_intrusive<NodeType>(i + 1, 0.0, 0.0, 0.0);
if (!fill_existing || i % 2 == 0) {
nodes_to_be_added.insert(nodes_to_be_added.end(), p_node);
} else {
existing_nodes.insert(existing_nodes.end(), p_node);
}
}
r_sub_sub_model_part.AddNodes(existing_nodes.begin(), existing_nodes.end());
state.ResumeTiming();

r_sub_sub_model_part.AddNodes(nodes_to_be_added.begin(), nodes_to_be_added.end());
}
}

static void BM_ModelPartAddNodesToSubSubFromRange2(benchmark::State& state) {
for (auto _ : state) {
const IndexType input_size = state.range(0);
const bool fill_existing = state.range(1) == 1;

state.PauseTiming();

auto model = Model();
auto& r_sub_sub_model_part = model.CreateModelPart("test").CreateSubModelPart("sub_test").CreateSubModelPart("sub_sub_test");

std::vector<ModelPart::NodeType::Pointer> nodes_to_be_added, existing_nodes;
if (!fill_existing) {
nodes_to_be_added.reserve(input_size);
} else {
nodes_to_be_added.reserve(input_size / 2 + 1);
existing_nodes.reserve(input_size / 2 + 1);
}

// doing it in reverse to make sure a proper sort is done always
for (IndexType i = 0; i < input_size; ++i) {
auto p_node = Kratos::make_intrusive<NodeType>(i + 1, 0.0, 0.0, 0.0);
if (!fill_existing || i % 2 == 0) {
nodes_to_be_added.insert(nodes_to_be_added.end(), p_node);
} else {
existing_nodes.insert(existing_nodes.end(), p_node);
}
}
r_sub_sub_model_part.GetRootModelPart().AddNodes(nodes_to_be_added.begin(), nodes_to_be_added.end());
r_sub_sub_model_part.GetRootModelPart().AddNodes(existing_nodes.begin(), existing_nodes.end());
state.ResumeTiming();

r_sub_sub_model_part.AddNodes(nodes_to_be_added.begin(), nodes_to_be_added.end());
}
}

static void BM_ModelPartAddNodesToSubSubFromRange3(benchmark::State& state) {
for (auto _ : state) {
const IndexType input_size = state.range(0);
const bool fill_existing = state.range(1) == 1;

state.PauseTiming();

auto model = Model();
auto& r_sub_sub_model_part = model.CreateModelPart("test").CreateSubModelPart("sub_test").CreateSubModelPart("sub_sub_test");

std::vector<ModelPart::NodeType::Pointer> nodes_to_be_added, existing_nodes;
if (!fill_existing) {
nodes_to_be_added.reserve(input_size);
} else {
nodes_to_be_added.reserve(input_size / 2 + 1);
existing_nodes.reserve(input_size / 2 + 1);
}

// doing it in reverse to make sure a proper sort is done always
for (IndexType i = 0; i < input_size; ++i) {
auto p_node = Kratos::make_intrusive<NodeType>(i + 1, 0.0, 0.0, 0.0);
if (!fill_existing || i % 2 == 0) {
nodes_to_be_added.insert(nodes_to_be_added.end(), p_node);
} else {
existing_nodes.insert(existing_nodes.end(), p_node);
}
}
r_sub_sub_model_part.GetRootModelPart().AddNodes(nodes_to_be_added.begin(), nodes_to_be_added.end());
r_sub_sub_model_part.GetRootModelPart().AddNodes(existing_nodes.begin(), existing_nodes.end());
state.ResumeTiming();

r_sub_sub_model_part.AddNodes(r_sub_sub_model_part.GetRootModelPart().NodesBegin(), r_sub_sub_model_part.GetRootModelPart().NodesEnd());
}
}

static void BM_ModelPartAddNodesToSubSubFromId1(benchmark::State& state) {
for (auto _ : state) {
const IndexType input_size = state.range(0);
const bool fill_existing = state.range(1) == 1;

state.PauseTiming();

auto model = Model();
auto& r_root_model_part = model.CreateModelPart("test");
auto& r_sub_sub_model_part = r_root_model_part.CreateSubModelPart("sub_test").CreateSubModelPart("sub_sub_test");

PointerVectorSet<ModelPart::NodeType, IndexedObject> nodes;
std::vector<IndexType> node_ids_to_add, existing_node_ids;
if (!fill_existing) {
node_ids_to_add.reserve(input_size);
} else {
node_ids_to_add.reserve(input_size / 2 + 1);
existing_node_ids.reserve(input_size / 2 + 1);
}

for (IndexType i = 0; i < input_size; ++i) {
nodes.insert(nodes.end(), Kratos::make_intrusive<NodeType>(i + 1, 0.0, 0.0, 0.0));
RiccardoRossi marked this conversation as resolved.
Show resolved Hide resolved
}

// doing it in reverse to make sure a proper sort is done always
for (IndexType i = input_size; i > 0; --i) {
if (!fill_existing || i % 2 == 0) {
node_ids_to_add.push_back(i);
} else {
existing_node_ids.push_back(i);
}
}
r_root_model_part.AddNodes(nodes.begin(), nodes.end());
r_sub_sub_model_part.AddNodes(existing_node_ids);
state.ResumeTiming();

r_sub_sub_model_part.AddNodes(node_ids_to_add);
}
}


// Register the function as a benchmark
BENCHMARK(BM_ModelPartCreateNewNode) -> RangeMultiplier(10) -> Range(1e2, 1e+6);
BENCHMARK(BM_ModelPartAddNodesToRootFromRange1) -> ArgsProduct({{100, 1000, 10000, 100000, 1000000}, {0, 1}});
BENCHMARK(BM_ModelPartAddNodesToRootFromRange2) -> ArgsProduct({{100, 1000, 10000, 100000, 1000000}, {0, 1}});
BENCHMARK(BM_ModelPartAddNodesToSubSubFromRange1) -> ArgsProduct({{100, 1000, 10000, 100000, 1000000}, {0, 1}});
BENCHMARK(BM_ModelPartAddNodesToSubSubFromRange2) -> ArgsProduct({{100, 1000, 10000, 100000, 1000000}, {0, 1}});
BENCHMARK(BM_ModelPartAddNodesToSubSubFromRange3) -> ArgsProduct({{100, 1000, 10000, 100000, 1000000}, {0, 1}});
BENCHMARK(BM_ModelPartAddNodesToSubSubFromId1) -> ArgsProduct({{100, 1000, 10000, 100000, 1000000}, {0, 1}});

} // namespace Kratos

BENCHMARK_MAIN();
41 changes: 36 additions & 5 deletions kratos/containers/pointer_vector_set.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <sstream>
#include <cstddef>
#include <utility>
#include <type_traits>

// External includes
#include <boost/iterator/indirect_iterator.hpp>
Expand Down Expand Up @@ -647,10 +648,23 @@ class PointerVectorSet final
template <class InputIterator>
void insert(InputIterator first, InputIterator last)
{
// first sorts the input iterators and make the input unique.
std::sort(first, last, CompareKey());
auto new_last = std::unique(first, last, EqualKeyTo());
SortedInsert(first, new_last);
if constexpr(std::is_assignable_v<decltype(*std::declval<InputIterator>()), decltype(*std::declval<InputIterator>())>) {
// first sorts the input iterators and make the input unique if the iterators are assignable
std::sort(first, last, CompareKey());
Copy link
Member

Choose a reason for hiding this comment

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

this means that you are changing the order of the input "in place", correct?

I am not sure if this is what you would expect. I am not sure of what is the praxis in this context. Do you have it clear?

auto new_last = std::unique(first, last, EqualKeyTo());
SortedInsert(first, new_last);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

sorry i cannot understand this ...

Copy link
Member

Choose a reason for hiding this comment

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

@Myself, this is for the case in which what i comment above is not possible ... still the comment above still holds

Copy link
Member Author

Choose a reason for hiding this comment

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

then i do not understand the role of mIds ...

mIds are storing the list of ids from a PVS when writing. So each rank will write its own local PVS ids which are sorted. The final list of mIds in the .h5 file won't be sorted, but it will be sorted for each partition in each rank. So when you read it also, each partition will read its portion of the mIds from the .h5 file whill will be an already sorted ids list. hence the rElements.insert(rElements.end(), p_elem) will have the hint which is valid, then PVS will do a push_back at the backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry i cannot understand this ...

This is to support iterators coming from const std::vector<NodeType::Pointer> for the insertion. There we need to sort and unique of the incoming iterators which is not allowed. So, you have to make a copy and do the sort and unique. This has additional overhead, but it cannot be avoided if we allow iterators coming from immutable containers.

Copy link
Member

Choose a reason for hiding this comment

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

yes i understand, however think at the following:

std::vector my_pointer_list ....
pvs.insert(my_pointer_list.begin(), my_pointer_list.end(); //it works
//but here my_pointer_list is ordered... i would not expect this. Is that something we accept? (open question...i am not sure of what would be a standardized behaviour)

indeed

const std::vector my_pointer_list .... //CONST!
//pvs.insert(my_pointer_list.begin(), my_pointer_list.end(); //will int principle not compile
pvs.insert(my_pointer_list.cbegin(), my_pointer_list.cend(); //i understand you will eventually remove the constness inside. Did i get it wrong?
//but here my_pointer_list is NOT ordered (ok, since it was const)

but now in the PVS will i have const pointers? if i do not, aren't we removing the constness wrongly?

to me this is a difficuly programming question, but i am really not clear about what will happen and OF WHAT SHOULD HAPPEN

Copy link
Member Author

@sunethwarna sunethwarna Dec 18, 2024

Choose a reason for hiding this comment

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

Hi @RiccardoRossi. This problem is fixed with the cpp standard way of doing this. The rvalue insertion methods are now implemented. So

std::vector my_pointer_list;
// following will not change the my_pointer_list
//but will incur a cost of copying the data in my_pointer_list to another vector.
pvs.insert(my_pointer_list.begin(), my_pointer_list.end()); 

If you are sure that you don't want the my_pointer_list anymore, then now you can use the following:

std::vector my_pointer_list;
// this will mutate the my_pointer_list avoiding a copy operation.
pvs.insert(std::move(my_pointer_list)); 

Is this acceptable? (The benchmark shows higher performance with rvalue methods as well.

There are unit tests added to the rvalue insertion as well.

// the iterators are not assignable, hence they may be coming from a const vector, So, we need to make a copy
// to do the sort and unique.
std::vector<TPointerType> temp;
temp.reserve(std::distance(first, last));
for (auto it = first; it != last; ++it) {
temp.push_back(GetPointer(it));
}
std::sort(temp.begin(), temp.end(), CompareKey());
auto new_last = std::unique(temp.begin(), temp.end(), EqualKeyTo());
SortedInsert(temp.begin(), new_last);
}
}

/**
Expand All @@ -667,6 +681,20 @@ class PointerVectorSet final
SortedInsert(first, last);
}

/**
* @brief Insert elements from another PointerVectorSet range.
* @details This function inserts element pointers from another PointerVectorSet range specified by first and last into the current set.
* Since, PointerVectorSet is assumed to be sorted and unique, the incoming PointerVectorSet is not
* sorted and made unique again. This will not insert any elements in the incoming set, if there exists an element with a key
* which is equal to an element's key in the input range.
* @param first Other PointerVectorSet starting iterator
* @param last Other PointerVectorSet ending iterator
*/
void insert(PointerVectorSet::iterator first, PointerVectorSet::iterator last)
{
SortedInsert(first, last);
}

void insert(const PointerVectorSet& rOther)
{
insert(rOther.begin(), rOther.end());
Expand Down Expand Up @@ -1199,7 +1227,10 @@ class PointerVectorSet final
// which is harder to guess, and cryptic. Hence, using the decltype.
using iterator_value_type = std::decay_t<decltype(*Iterator)>;

if constexpr(std::is_same_v<iterator_value_type, std::remove_cv_t<TPointerType>>) {
if constexpr(std::is_same_v<TIteratorType, iterator> || std::is_same_v<TIteratorType, reverse_iterator>) {
Copy link
Member

Choose a reason for hiding this comment

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

@roigcarlo can you take a look into GetPointer and GetReference? these are difficult functions and i am quite afraid of their implications

// if the TIteratorType is of boost::indirect_iterator type, then we can get the pointer by dereferencing.
return *(Iterator.base());
} else if constexpr(std::is_same_v<iterator_value_type, std::remove_cv_t<TPointerType>>) {
// this supports any type of pointers
return *Iterator;
} else if constexpr(std::is_same_v<iterator_value_type, std::remove_cv_t<value_type>> && std::is_same_v<TPointerType, Kratos::intrusive_ptr<std::decay_t<decltype(*Iterator)>>>) {
Expand Down
Loading
Loading