BMesh: Optimize copying attributes from many elements at once #115824

Hans Goudey · 2023-12-06T04:36:08+01:00

Hans Goudey commented

2023-12-06 04:36:08 +01:00

Create a minimal structure that contains the instructions necessary to
copy from one custom data format to another. This structure is similar
to the one used in dfacaf4f40. It should have been used in
9175d9b7c2, which instead introduced quadratic performance
relative to the number of layers when copying every element.

In this commit, copying the entire mesh and adding new custom data
are explicitly changed to use the new map to speed up copying many
elements at a time.

The non-map attribute copy functions are also changed to check for when
the source and result BMeshes are the same. In that case it's much
faster to call the "same format" function from 9175d9b7c2.

For numbers, the timings are arbitrarily influenced by how many layers
I add in my testing. With 50 or so layers, a 10x difference is easily
observable though.

Fixes #115776 Create a minimal structure that contains the instructions necessary to copy from one custom data format to another. This structure is similar to the one used in dfacaf4f409287baa1c8. It should have been used in 9175d9b7c27b191ea94a, which instead introduced quadratic performance relative to the number of layers when copying every element. In this commit, copying the entire mesh and adding new custom data are explicitly changed to use the new map to speed up copying many elements at a time. The non-map attribute copy functions are also changed to check for when the source and result BMeshes are the same. In that case it's much faster to call the "same format" function from 9175d9b7c27b191ea94a. For numbers, the timings are arbitrarily influenced by how many layers I add in my testing. With 50 or so layers, a 10x difference is easily observable though.

Hans Goudey added 1 commit 2023-12-06 04:36:17 +01:00

119a9bba4e BMesh: Optimize copying attributes from many elements at once

Fixes #115776

Create a minimal structure that contains the instructions necessary to
copy from one custom data format to another. This structure is similar
to the one used in dfacaf4f40. It should have been used in
9175d9b7c2, which instead introduced quadratic performance
relative to the number of layers when copying every element.

A few areas in BMesh are changed to use this map:
- Copying the entire mesh
- Adding a custom data layer (attribute)

The non-map attribute copy functions are also changed to check for when
the source and result BMeshes are the same. In that case it's much
faster to call the "same format" function from 9175d9b7c2.

For numbers, the timings are arbitrarily influenced by how many layers
I add in my testing. With many very many layers, a 10x difference is
easily observable though.

Hans Goudey added this to the Modeling project 2023-12-06 04:36:29 +01:00

Hans Goudey requested review from Campbell Barton 2023-12-06 04:36:42 +01:00

Campbell Barton requested changes 2023-12-07 12:16:40 +01:00

Campbell Barton left a comment

Generally fine, only request is not to expand existing cases where a single function call ends up in-lining normal/material copying. While there aren't many cases I think it's reasonable we keep something like BM_elem_attrs_copy_ex so Python logic doesn't have to inline this.

If we want to make using a map the more obvious choice we could even remove _with_map functions and have _without_map(..) / _no_map(..) with warnings that these functions should be avoided if at all possible.

Generally fine, only request is not to expand existing cases where a single function call ends up in-lining normal/material copying. While there aren't many cases I think it's reasonable we keep something like `BM_elem_attrs_copy_ex` so Python logic doesn't have to inline this. If we want to make using a map the more obvious choice we could even remove `_with_map` functions and have `_without_map(..)` / `_no_map(..)` with warnings that these functions should be avoided if at all possible.

source/blender/blenkernel/BKE_customdata.hh Outdated

						
				@ -360,0 +391,4 @@

				                                                   const CustomData &dst,

				                                                   eCustomDataMask mask_exclude = 0);

				void CustomData_bmesh_copy_block_with_map(const BMCustomDataCopyMap &map,

Campbell Barton commented

2023-12-06 12:03:31 +01:00

Prefer to order the helper argument (map in this case) last, since it's the convention to have containers first bm in this case.

Prefer to order the helper argument (map in this case) last, since it's the convention to have containers first `bm` in this case.

HooglyBoogly marked this conversation as resolved

source/blender/python/bmesh/bmesh_py_types.cc Outdated

						
				@ -1498,0 +1503,4 @@

				      BMVert *dst = reinterpret_cast<BMVert *>(self->ele);

				      CustomData_bmesh_copy_block(

				          &value->bm->vdata, &self->bm->vdata, src->head.data, &dst->head.data);

				      copy_v3_v3(dst->no, src->no);

Campbell Barton commented

2023-12-06 12:00:26 +01:00

Prefer use a utility function for each type instead of copying each type & it's members here.

HooglyBoogly marked this conversation as resolved

Hans Goudey added 3 commits 2023-12-07 16:00:39 +01:00

5b7989b8db Merge branch 'main' into bmesh-cd-copy-performance-fix

9acb056f86 Order map arguments after containers

22b40965da Don't inline "low level" copying in BMesh python code

Hans Goudey commented

2023-12-07 16:01:46 +01:00

If we want to make using a map the more obvious choice we could even remove _with_map functions and have _without_map(..) / _no_map(..) with warnings that these functions should be avoided if at all possible.

Not sure that's necessary for now. I think there's still room to make copying without a prebuilt map faster. So maybe it won't be that necessary long term, though it will always be worth it for things like reallocating all custom data blocks. I'd almost prefer to just use one overloaded name for that. But I left it as is now.

>If we want to make using a map the more obvious choice we could even remove _with_map functions and have _without_map(..) / _no_map(..) with warnings that these functions should be avoided if at all possible. Not sure that's necessary for now. I think there's still room to make copying without a prebuilt map faster. So maybe it won't be that necessary long term, though it will always be worth it for things like reallocating all custom data blocks. I'd almost prefer to just use one overloaded name for that. But I left it as is now.

Hans Goudey requested review from Campbell Barton 2023-12-07 16:01:53 +01:00

Campbell Barton approved these changes 2023-12-07 21:53:45 +01:00

Campbell Barton left a comment

Functions that have a _with_map() version should note in doc-string the with_map() version should be preferred when calling multiple times.
As the map argument is cache (helper information), I'd rather it be last, so non-map functions use the same leading arguments.

Doesn't require an extra iteration, accepting.

- Functions that have a `_with_map()` version should note in doc-string the `with_map()` version should be preferred when calling multiple times. - As the map argument is cache (helper information), I'd rather it be last, so non-map functions use the same leading arguments. Doesn't require an extra iteration, accepting.

Hans Goudey commented

2023-12-07 22:54:37 +01:00

note in doc-string the with_map() version should be preferred when calling multiple times.

Good idea.

As the map argument is cache (helper information), I'd rather it be last

I don't agree with that as a general rule, but I don't care much in this case, so I'll change it so we can move on.

>note in doc-string the with_map() version should be preferred when calling multiple times. Good idea. >As the map argument is cache (helper information), I'd rather it be last I don't agree with that as a general rule, but I don't care much in this case, so I'll change it so we can move on.

Hans Goudey commented

2023-12-07 23:23:16 +01:00

As the map argument is cache (helper information), I'd rather it be last, so non-map functions use the same leading arguments.

Actually I do care here :P The reason to have it not last is all of the constant/global arguments come before the arguments that change for every copy/iteration. I think that's a much better general rule ("global/larger scope arguments first") than "caches last". Are you okay with that?

>As the map argument is cache (helper information), I'd rather it be last, so non-map functions use the same leading arguments. Actually I do care here :P The reason to have it not last is all of the constant/global arguments come before the arguments that change for every copy/iteration. I think that's a much better general rule ("global/larger scope arguments first") than "caches last". Are you okay with that?

Hans Goudey added 2 commits 2023-12-07 23:26:58 +01:00

ed3170c1ba Merge branch 'main' into bmesh-cd-copy-performance-fix

496f823998 Improve comments

Campbell Barton commented

2023-12-07 23:49:08 +01:00

As the map argument is cache (helper information), I'd rather it be last, so non-map functions use the same leading arguments.

Actually I do care here :P The reason to have it not last is all of the constant/global arguments come before the arguments that change for every copy/iteration. I think that's a much better general rule ("global/larger scope arguments first") than "caches last". Are you okay with that?

It's not that cache should have a particular argument location, it's that this is extra/optional information
(optional in the sense there is a function which doesn't require it, and this is an extended version of that function).

foo(a, b, c)
foo_with_cache(a, b, c, cache)

Reads more logically to me than:

foo(a, b, c)
foo_with_cache(a, cache, b, c)

> >As the map argument is cache (helper information), I'd rather it be last, so non-map functions use the same leading arguments. > > Actually I do care here :P The reason to have it not last is all of the constant/global arguments come before the arguments that change for every copy/iteration. I think that's a much better general rule ("global/larger scope arguments first") than "caches last". Are you okay with that? It's not that *cache* should have a particular argument location, it's that this is extra/optional information (optional in the sense there is a function which doesn't require it, and this is an extended version of that function). ``` foo(a, b, c) foo_with_cache(a, b, c, cache) ``` Reads more logically to me than: ``` foo(a, b, c) foo_with_cache(a, cache, b, c) ```

Hans Goudey commented

2023-12-08 01:28:54 +01:00

I don't think that logic scales. To me, the existence of a different version of a function with different arguments doesn't mean those arguments should go last. They should go in the logical order in the context of that version of the function. And we already have a fairly well established ordering for that sort of "global" vs. "per iteration" data.

Hans Goudey added 1 commit 2023-12-08 03:48:33 +01:00

673664bf54 Merge branch 'main' into bmesh-cd-copy-performance-fix

Campbell Barton commented

2023-12-08 07:00:03 +01:00

I don't think that logic scales. To me, the existence of a different version of a function with different arguments doesn't mean those arguments should go last. They should go in the logical order in the context of that version of the function. And we already have a fairly well established ordering for that sort of "global" vs. "per iteration" data.

This is the current convention for _ex() although that means (extended), we don't have such clear conventions for _with_*(..) suffix functions, but it reads strangely to me to re-order arguments.

Reading over the patch there are two functions called CustomData_bmesh_copy_block(..) one that takes a single mesh which doesn't need a mapping, and another that calculates and passes a map to CustomData_bmesh_copy_block_with_map(..).
The problem with this is one of these functions we should really avoid where possible but it's got the same name as a function that doesn't have a performance problem. Checking over the code it's used ~17 or so places, many of these cases map can be calculated and reused.

Suggestion:

Rename CustomData_bmesh_copy_block_with_map(..) to CustomData_bmesh_copy_block.
Rename the version of CustomData_bmesh_copy_block that generates it's a map to _without_map(..), _no_map(..) or similar.
Keep argument order as is.

This means callers are aware there is a map that should be reused, and using a map is the default.

Note, we could even remove the non-map version of the function however it's used enough right now that it's not so convenient at the moment.

> I don't think that logic scales. To me, the existence of a different version of a function with different arguments doesn't mean those arguments should go last. They should go in the logical order in the context of that version of the function. And we already have a fairly well established ordering for that sort of "global" vs. "per iteration" data. This is the current convention for `_ex()` although that means (extended), we don't have such clear conventions for `_with_*(..)` suffix functions, but it reads strangely to me to re-order arguments. Reading over the patch there are two functions called `CustomData_bmesh_copy_block(..)` one that takes a single mesh which doesn't need a mapping, and another that calculates and passes a map to `CustomData_bmesh_copy_block_with_map(..)`. The problem with this is one of these functions we should really avoid where possible but it's got the same name as a function that doesn't have a performance problem. Checking over the code it's used ~17 or so places, many of these cases map can be calculated and reused. Suggestion: - Rename `CustomData_bmesh_copy_block_with_map(..)` to `CustomData_bmesh_copy_block`. - Rename the version of `CustomData_bmesh_copy_block` that generates it's a map to `_without_map(..)`, `_no_map(..)` or similar. - Keep argument order as is. This means callers are aware there is a map that should be reused, and using a map is the default. --- Note, we *could* even remove the non-map version of the function however it's used enough right now that it's not so convenient at the moment.

Hans Goudey added 2 commits 2023-12-08 15:34:55 +01:00

4110ed7ec9 Merge branch 'main' into bmesh-cd-copy-performance-fix

57c9c6fae8 Remove "with map" suffix and remove old function

Hans Goudey commented

2023-12-08 15:35:41 +01:00

I removed the non "with map" version for copying between two different CustomData formats and removed the "with map" suffix. I think this state is fine now, should make both of us happy :)

I removed the non "with map" version for copying between two different `CustomData` formats and removed the "with map" suffix. I think this state is fine now, should make both of us happy :)

👍 1

Campbell Barton approved these changes 2023-12-09 04:45:00 +01:00

Campbell Barton commented

2023-12-09 04:48:14 +01:00

Much better although I´m still uneasy that a 100x slower function is named in a way that doesn´t make this down-side explicit, longer term it might be good to phase out the slow version entirely.

Hans Goudey merged commit 0b1049b41d into main

2023-12-09 05:37:47 +01:00

Hans Goudey deleted branch bmesh-cd-copy-performance-fix

2023-12-09 05:37:48 +01:00

Hans Goudey referenced this issue from a commit

2023-12-09 05:37:50 +01:00

BMesh: Optimize copying attributes from many elements at once

Philipp Oeser commented

2023-12-10 14:34:50 +01:00

Was stumbling over this, shouldnt it be the following?

diff --git a/source/blender/bmesh/intern/bmesh_construct.cc b/source/blender/bmesh/intern/bmesh_construct.cc
index ff7f7ab5c24..858b3a940ec 100644
--- a/source/blender/bmesh/intern/bmesh_construct.cc
+++ b/source/blender/bmesh/intern/bmesh_construct.cc
@@ -591,7 +591,7 @@ BMesh *BM_mesh_copy(BMesh *bm_old)
                            e,
                            BM_CREATE_SKIP_CD);
 
-    CustomData_bmesh_copy_block(bm_new->edata, vert_map, e->head.data, &e_new->head.data);
+    CustomData_bmesh_copy_block(bm_new->edata, edge_map, e->head.data, &e_new->head.data);
     e_new->head.hflag = e->head.hflag; /* low level! don't do this for normal api use */
     etable[i] = e_new;
     BM_elem_index_set(e, i);     /* set_inline */

Was stumbling over this, shouldnt it be the following? ```diff diff --git a/source/blender/bmesh/intern/bmesh_construct.cc b/source/blender/bmesh/intern/bmesh_construct.cc index ff7f7ab5c24..858b3a940ec 100644 --- a/source/blender/bmesh/intern/bmesh_construct.cc +++ b/source/blender/bmesh/intern/bmesh_construct.cc @@ -591,7 +591,7 @@ BMesh *BM_mesh_copy(BMesh *bm_old) e, BM_CREATE_SKIP_CD); - CustomData_bmesh_copy_block(bm_new->edata, vert_map, e->head.data, &e_new->head.data); + CustomData_bmesh_copy_block(bm_new->edata, edge_map, e->head.data, &e_new->head.data); e_new->head.hflag = e->head.hflag; /* low level! don't do this for normal api use */ etable[i] = e_new; BM_elem_index_set(e, i); /* set_inline */ ```

Hans Goudey commented

2023-12-11 00:52:42 +01:00

Thanks, resolved in fb5d03b5ba

Thanks, resolved in fb5d03b5baae96d3cea78273702940f4765a9f48

Brian Savery (AMD) referenced this issue from a commit

2023-12-28 20:07:44 +01:00

BMesh: Optimize copying attributes from many elements at once

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

BMesh: Optimize copying attributes from many elements at once #115824