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

Merged
Hans Goudey merged 9 commits from HooglyBoogly/blender:bmesh-cd-copy-performance-fix into main 2023-12-09 05:37:47 +01:00
Member

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.

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
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
Owner

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.
@ -360,0 +391,4 @@
const CustomData &dst,
eCustomDataMask mask_exclude = 0);
void CustomData_bmesh_copy_block_with_map(const BMCustomDataCopyMap &map,

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

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

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
Author
Member

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
Owner
  • 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.
Author
Member

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.
Author
Member

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

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) ```
Author
Member

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.

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

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
Author
Member

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 :)
Campbell Barton approved these changes 2023-12-09 04:45:00 +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.

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
Member

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 */ ```
Author
Member

Thanks, resolved in fb5d03b5ba

Thanks, resolved in fb5d03b5baae96d3cea78273702940f4765a9f48
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#115824
No description provided.