[Core] Remove push_back from ModelPart #12903

sunethwarna · 2024-12-05T06:10:14Z

📝 Description
This PR removes uses of PointerVectorSet::push_back method from the ModelPart. Followings are the modifications.

When removing entities, erase_count was not calculated correctly, hence the reserved size is wrong. This was fixed
Introduced Container class which can retireve the correct PointerVectorSet container from a given Mesh, and its std::string for nice error message output.
Refactored the methods as much as possible using templates. [Unfortunatly, Geometries container cannot be templated since it is in the ModelPart, and all the other PointerVectorSet containers are in the Mesh].

🆕 Changelog

Removes PointerVectorSet::push_back uses from the ModelPart
Fixes bugs in erase count calculation and size reservation when removing Nodes/Conditions/Elements/MasterSlaveConstraints.
Added Container class with templates to retrieve given PointerVectorSet container from a mesh.

loumalouomega · 2024-12-05T07:43:03Z

Can you use the benchmark to see if there is an improvement?

loumalouomega · 2024-12-05T07:44:12Z

I will further review once the CI passes

RiccardoRossi · 2024-12-05T09:27:47Z

kratos/includes/model_part.h

+    template<class TReturnValueType>
+    struct ReferenceGetter
+    {
+        template<class TInputValueType>


wow, this needs some explanation to me.

and the GetPointer even more...

comments are added for the use case.

RiccardoRossi · 2024-12-09T10:45:02Z

kratos/includes/model_part.h

+            });
+
+            //now add to the root model part
+            Container<TContainerType>::GetContainer(p_root_model_part->GetMesh()).insert(begin, end);


by adding always to the root all of them, a reserve is done in the pointer vector set on the root which increases its size

On the other hand, it is not always good to call shrink_to_fit as well. In the new design, if a user tries to add a range to a submodel part which is a sub-set of the one of the parent model parts, then it will not reserve the parent model parts which a super set of the range being added. I think this solves this problem partially.

In any case, I can expose the method shrink_to_fit, but I think this should be done if required only.

sorry, but then with the current implementation the size will always grow?

size will not grow, but the capcity will only grow if the current_size + new_item < current_capcity. So, it may grow or it may not. But it will never keep on growing unless the size keeps on growings. This is further optimized by chekcing for subranges, if the addition is of a subrange, then the size, nor the capacity will increase. It will simply ignore the addition.

There is a test also added to check this.

RiccardoRossi

i would say this is fine as far as i can see it .

I leave @KratosMultiphysics/technical-committee for approval

RiccardoRossi · 2024-12-18T13:50:30Z

well, two curiosities (the second more important than the first):

1 - from what i can see threading does not provide any benefit in the benchmarks (nor any deterioration). It is what you expected, correct?
2 - the problem related to the growing size if you were pushing back many times without doing shrink_to_fit is it solveD?

sunethwarna · 2024-12-18T17:43:51Z

@RiccardoRossi followings are my answers:

Regarding the multi threading -> we don't have much methods which are using shared memory parallelism in model part, so may be that or I am doing the benchmarking wrongly with shared memory parallelism. I just change the OMP_NUM_THREADS and run the benchmarks. May be @loumalouomega knows better or can shed some light to this?
As far as I see it is fixed, because if you now keep on adding the same container or subset of that container to the same container, it will check for subsets and ignore fully the insertion. If not it will increase the capacity which will be `current_size + new_potential_items". So it may or may not increase the capacity because it is always based on the current size of the container, not on the current capacity of the container.

loumalouomega · 2024-12-18T17:49:15Z

@RiccardoRossi followings are my answers:

1. Regarding the multi threading -> we don't have much methods which are using shared memory parallelism in model part, so may be that or I am doing the benchmarking wrongly with shared memory parallelism. I just change the `OMP_NUM_THREADS` and run the benchmarks. May be @loumalouomega knows better or can shed some light to this?

That should be OK if tests are global enough.

matekelemen

Nice, thx @sunethwarna.

I have some mostly doc-related and aesthetical comments, but also request about a test. You mustn't check the container once you moved it.

matekelemen · 2024-12-19T13:27:57Z

kratos/containers/pointer_vector_set.h

+    /**
+     * @brief Inserts elements from a r_value container.
+     * @details This method is used to insert all values from an r valued container.
+     *          This mutates the r valued input container to be sorted and unique if it is not the type of PointerVectorSet.
+     *          If it is the type of the PointerVectorSet, then it doesn't mutate the input container.
+     *
+     * @warning The move assumes the ownership of the container, hence the the rContainer may be mutated. if the TContainer is of type std::shared_ptr or intrusive_ptr
+     *          then, there will always be a null pointer at the end past position of the unique sorted list since the std::unique uses the move assignment operator
+     *          within its algorithm. Therefore, the new_last created here will have the correct pointer to the object, and the rContainer will have a nullptr in the
+     *          corresponding place. Therefore, if this method is called once, and if it used with a container type which is not PointerVectorSet, please do not use
+     *          the input rContainer anymore. It will segfault unless the PointerVectorSet is created with raw pointers. [This is because since c++11 std::unique uses the move assignment operator]
+     * @tparam TContainer
+     * @param rContainer
+     */


The user doesn't have to know all these details. In fact they must not know in my opinion. All they should be aware of, is that they moved their container into the function, so they cannot use it anymore.

Suggested change

/**

* @brief Inserts elements from a r_value container.

* @details This method is used to insert all values from an r valued container.

* This mutates the r valued input container to be sorted and unique if it is not the type of PointerVectorSet.

* If it is the type of the PointerVectorSet, then it doesn't mutate the input container.

*

* @warning The move assumes the ownership of the container, hence the the rContainer may be mutated. if the TContainer is of type std::shared_ptr or intrusive_ptr

* then, there will always be a null pointer at the end past position of the unique sorted list since the std::unique uses the move assignment operator

* within its algorithm. Therefore, the new_last created here will have the correct pointer to the object, and the rContainer will have a nullptr in the

* corresponding place. Therefore, if this method is called once, and if it used with a container type which is not PointerVectorSet, please do not use

* the input rContainer anymore. It will segfault unless the PointerVectorSet is created with raw pointers. [This is because since c++11 std::unique uses the move assignment operator]

* @tparam TContainer

* @param rContainer

*/

/**

* @brief Inserts items from an @a rvalue container.

* @details This method takes ownership of the input container, and inserts all its items into @ref PointerVectorSet.

*

* @warning The move assumes the ownership of the container, hence the rContainer may be mutated.

* @tparam TContainer

* @param rContainer

*/

I would say, lets keep this detailed description so that we will find it easier to understand. 99% of the users will never look at the PVS, they will stop at the ModelPart.

kratos/containers/pointer_vector_set.h

matekelemen · 2024-12-19T13:37:57Z

kratos/sources/model_part.cpp

+template<class TContainerType>
+void RemoveEntities(
+    ModelPart::MeshType& rMesh,
+    const Flags& rIdentifierFlag)


I'd just pass a predicate lambda to select which items to remove, instead hard-coding flags to be checked. It'd be much more flexible and similar to the standard lib's interface. Your call though.

this one is already there...we could add a new one

Not in this PR. Let's do it in a different PR if someone requests it, or if we see some performance improvements by it.

kratos/includes/model_part.h

matekelemen · 2024-12-20T08:02:45Z

kratos/includes/model_part.h

+                    if (GeometryType::HasSameGeometryType(r_geometry, *it_found)) { // Check the geometry type and connectivities
+                        for (IndexType i_pt = 0; i_pt < r_geometry.PointsNumber(); ++i_pt) {
+                            KRATOS_ERROR_IF((r_geometry)[i_pt].Id() != (*it_found)[i_pt].Id()) << "Attempting to add a new geometry with Id: " << r_geometry.Id() << ". A same type geometry with same Id but different connectivities already exists." << std::endl;
+                        }
+                    } else if(&(*it_found) != &r_geometry) { // Check if the pointee coincides
+                        KRATOS_ERROR << "Attempting to add a new geometry with Id: " << it_found->Id() << ". A different geometry with the same Id already exists." << std::endl;
+                    }


@sunethwarna I know you didn't write this and it's the current behavior, but I have to mention that I really don't like that we have to support duplicate insertions of the same geometry.

Apart from code complexity issues, this really does have a pretty significant performance impact on insertion. I don't get why we're nitpicking over other minor issues when we validate every single geometry inserted into the model part.

i would say this is a different discussion ... i really do not think we should chaneg any behaviour in this PR

I agree with @RiccardoRossi . This PR tries to keep the behaviour changes to the minimum.

kratos/includes/model_part.h

matekelemen · 2024-12-20T08:09:17Z

kratos/tests/cpp_tests/containers/test_pointer_vector_set.cpp

@@ -150,6 +150,18 @@ KRATOS_TEST_CASE_IN_SUITE(PointerVectorSetInsert4, KratosCoreFastSuite)
    tmp.push_back(p_element_3_copy);
    test_container.insert(tmp.begin(), tmp.end());

+    // check whether the original tmp is not touched.
+    auto temp_itr = tmp.begin();
+    KRATOS_EXPECT_EQ(&**(temp_itr++), &*p_element_2);


&**(temp_itr++)
oh god :D

hehe.... It it a test, so people can bear it :)

kratos/tests/cpp_tests/containers/test_pointer_vector_set.cpp

kratos/containers/pointer_vector_set.h

RiccardoRossi · 2024-12-20T08:30:13Z

kratos/includes/model_part.h

+                    if (GeometryType::HasSameGeometryType(r_geometry, *it_found)) { // Check the geometry type and connectivities
+                        for (IndexType i_pt = 0; i_pt < r_geometry.PointsNumber(); ++i_pt) {
+                            KRATOS_ERROR_IF((r_geometry)[i_pt].Id() != (*it_found)[i_pt].Id()) << "Attempting to add a new geometry with Id: " << r_geometry.Id() << ". A same type geometry with same Id but different connectivities already exists." << std::endl;
+                        }
+                    } else if(&(*it_found) != &r_geometry) { // Check if the pointee coincides
+                        KRATOS_ERROR << "Attempting to add a new geometry with Id: " << it_found->Id() << ". A different geometry with the same Id already exists." << std::endl;
+                    }


i would say this is a different discussion ... i really do not think we should chaneg any behaviour in this PR

RiccardoRossi · 2024-12-20T08:31:45Z

kratos/sources/model_part.cpp

+template<class TContainerType>
+void RemoveEntities(
+    ModelPart::MeshType& rMesh,
+    const Flags& rIdentifierFlag)


this one is already there...we could add a new one

kratos/tests/cpp_tests/containers/test_pointer_vector_set.cpp

RiccardoRossi · 2024-12-20T08:37:07Z

kratos/sources/model_part.cpp

+        // when insertion is done. since we dont use the entities anymore we can use the move operator here.
+        // since aux is an empty container. it will call push_back in the insert.
+        container_type aux;
+        aux.insert(std::move(entities));


why do we need the "container_type" at all here? cannot we move directly the entities in line 78?

No, we cannot. Beacuse, the std::move in line 73 wll sort the entities and will make them unique. Thereofre, line 78 will only be doing the insertion without sorting and making unique. Otherwise, it needs to sort and make it to unique when adding to each of the parent model parts which is redundant.

RiccardoRossi · 2024-12-20T08:38:00Z

kratos/sources/model_part.cpp

+        if (i_entity->IsNot(rIdentifierFlag)) {
+            // we can safely insert them at the end with the correct hint here since, the original r_mesh
+            // is sorted and unique.
+            r_container.insert(r_container.end(), std::move(*(i_entity.base())));


i would say this std::move is not needed ... it will be moved automatically

I think it is cleaner to explicitly say it so whoever reads the code knows what is happening without having to dive through the code.

kratos/includes/model_part.h

matekelemen · 2025-01-02T13:40:18Z

kratos/tests/cpp_tests/sources/test_model_part.cpp

+        }
+
+        KRATOS_EXPECT_EQ(r_model_part.Nodes().size(), 16);
+        KRATOS_EXPECT_EQ(r_model_part.Nodes().capacity(), 16);


No wonder this fails in the CI. Memory management is up to the standard lib's implementation. The only guaranteed way of enforcing a specific capacity is with shrink_to_fit and you haven't called that anywhere.

I'd just remove capacity tests altogether.

I have changed to check for the change in capacity and used shrink_to_fit to align memory allocations in different compilers.

Well, I don't think that checking shrink_to_fit is necessary but it also doesn't hurt. The other change (checking against an earlier capacity) is better in my opinion.

sunethwarna · 2025-01-03T10:08:52Z

Hi all, i think this PR has improved a lot and is ready for the final review. :)

matekelemen

👍 from my side

sunethwarna · 2025-01-04T13:00:44Z

pinging @roigcarlo @rubenzorrilla @RiccardoRossi @pooyan-dadvand @loumalouomega

loumalouomega · 2025-01-07T08:45:58Z

Not blocking from my side

roigcarlo

Also think is ready. Just a couple of minor comments.

Also, I really think that not all cases where NodesContainerType etc... are being misused are covered here (mainly apps that are not on the CI)

So please double check that 🙏

roigcarlo · 2025-01-14T09:14:58Z

kratos/containers/pointer_vector_set.h

+        auto new_last = std::unique(temp.begin(), temp.end(), EqualKeyTo());
+        SortedInsert(temp.begin(), new_last);
+
+        // KRATOS_ERROR << "HELLO THERE";


Remove this.

roigcarlo · 2025-01-14T09:21:53Z

kratos/containers/pointer_vector_set.h

@@ -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>) {


I think we had this same discussion in another PR o maybe it was this one. Not the most exciting code imo but is a perfectly valid case just to remove part of the boilerplate from boost.

Otherwise we would need to have these functions n-plicated for every trait combination of the iterator (or a stupidly long list of if constexpr).

kratos/containers/pointer_vector_set.h

roigcarlo · 2025-01-14T09:50:49Z

applications/HDF5Application/custom_io/hdf5_connectivities_data.cpp

+        // 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);


I tend to lean with @sunethwarna point of view here. We just assume that id's are ordered here, or at least provide a mechanism to sort them before reaching this points. Not only the code is easier to read and do not need sorts in between, when we move to C++20 we can replace this with something like:

... auto entities = std::views::iota(0,num_new_elems) | std::views::transform([&](int i) -> ElementType::Pointer { for (unsigned j = 0; j < geometry_size; j++) { const int node_id = mConnectivities(i, j); nodes(j) = rNodes(node_id); } return p_elem = r_elem.Create(mIds[i], nodes, rProperties(mPropertiesIds[i])); }); rElements.insert(entities.begin(), entities.end(), entities.end())

Which ensures that the view is inserted directly into rElements and remove the need for a temporal container with random access or the need to insert 1 by 1, which means that will be faster.

roigcarlo

Approving on behalf of the @KratosMultiphysics/technical-committee ( but please remove the comments :) )

sunethwarna added 2 commits December 5, 2024 07:04

add additional insert and typecheck to PVS

dd02fc8

remove push_backs from ModelPart

2bf1e96

sunethwarna requested a review from a team as a code owner December 5, 2024 06:10

sunethwarna requested review from RiccardoRossi, pooyan-dadvand, roigcarlo, rubenzorrilla, philbucher, loumalouomega and matekelemen December 5, 2024 06:13

sunethwarna self-assigned this Dec 5, 2024

sunethwarna added Kratos Core C++ labels Dec 5, 2024

sunethwarna added 3 commits December 5, 2024 07:16

include reduction_utils

2c5d23f

minor

d5521ba

func name change

2f72e0c

RiccardoRossi reviewed Dec 9, 2024

View reviewed changes

sunethwarna added 9 commits December 10, 2024 12:49

fix embedded_skin_utility for Id Change

1e98ea3

allow insertion from const vectors

9c05b31

allow insertion from parent entity ranges

abbce15

fix for iterator invalidation

71ae35f

add comments

6d34602

minor signature modification

f1c4a6f

fix assign_ms_const_to_neigh_utils

35c1874

fix error msg tests

13e87e7

fix hdf5

a50fd2b

sunethwarna requested a review from a team as a code owner December 11, 2024 04:19

generalize addition from ids

8a42759

sunethwarna added 7 commits December 18, 2024 10:24

allow rvalue containers in pvs insert

fedc6b3

allow rvalue containers in model part

d642295

tests for rvalue containers in PVS

ee29e90

more performance improvements

71e3074

fix tests

1024156

minor

a9e8219

bug fix

abfdcab

sunethwarna requested a review from RiccardoRossi December 18, 2024 11:14

RiccardoRossi reviewed Dec 18, 2024

View reviewed changes

matekelemen reviewed Dec 20, 2024

View reviewed changes

RiccardoRossi reviewed Dec 20, 2024

View reviewed changes

sunethwarna added 2 commits January 2, 2025 11:43

fix sub range addition

a00437f

add a test to subrange addition

36fd864

matekelemen reviewed Jan 2, 2025

View reviewed changes

kratos/includes/model_part.h Outdated Show resolved Hide resolved

fix test

88a5f51

sunethwarna requested review from matekelemen and RiccardoRossi January 2, 2025 11:03

matekelemen reviewed Jan 2, 2025

View reviewed changes

sunethwarna added 2 commits January 2, 2025 15:43

expose shrink_to_fit

22ff3e1

change test

99aca1d

sunethwarna requested a review from matekelemen January 2, 2025 23:16

matekelemen reviewed Jan 3, 2025

View reviewed changes

roigcarlo reviewed Jan 14, 2025

View reviewed changes

roigcarlo approved these changes Jan 21, 2025

View reviewed changes

[Core] Remove push_back from ModelPart #12903

Are you sure you want to change the base?

[Core] Remove push_back from ModelPart #12903

Conversation

sunethwarna commented Dec 5, 2024 • edited Loading

loumalouomega commented Dec 5, 2024

loumalouomega commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RiccardoRossi left a comment

Choose a reason for hiding this comment

RiccardoRossi commented Dec 18, 2024

sunethwarna commented Dec 18, 2024

loumalouomega commented Dec 18, 2024

matekelemen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna commented Jan 3, 2025

matekelemen left a comment

Choose a reason for hiding this comment

sunethwarna commented Jan 4, 2025

loumalouomega commented Jan 7, 2025

roigcarlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roigcarlo Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

roigcarlo left a comment

Choose a reason for hiding this comment

sunethwarna commented Dec 5, 2024 •

edited

Loading

sunethwarna Dec 10, 2024 •

edited

Loading

sunethwarna Jan 2, 2025 •

edited

Loading

sunethwarna Jan 2, 2025 •

edited

Loading

sunethwarna Jan 2, 2025 •

edited

Loading

roigcarlo Jan 14, 2025 •

edited

Loading