Page MenuHome
Differential D11337

Depsgraph: remove redundant mesh data duplication in edit-mode
ClosedPublic
Actions

Authored by Campbell Barton (campbellbarton) on May 21 2021, 10:23 AM.
Tags
Subscribers
Erick Tukuniata (erickblender)
Germano Cavalcante (mano-wii)
Tokens
"Love" token, awarded by kursadk."Love" token, awarded by Tetone."Love" token, awarded by gilberto_rodrigues."Like" token, awarded by erickblender.

Details

Reviewers
Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)
Summary

This resolves a bottleneck where every update while transforming
copied the entire mesh data-block, which isn't needed as the edit-mesh
is the source of the data being edited.

Testing shows a significant overall speedup when transforming:

  • ~1.5x with a subdivided cube 1.5 million vertices.
  • ~3.0x with the spring mesh (edit-mode with modifiers disabled, duplicated 10x to drop performance).

Notes:

Alternatives:

Some alternative solutions I considered, although I'm not so experienced with the depsgraph, so it's possible I missed something.

  • Copy-on-write could define components for updating e.g. shared-pointers, runtime-data, id-data. This would allow copy-on-write updates to be left enabled, while avoiding expensive operations.
  • Recalculating geometry could never do copy-on-write, instead we could require the caller to pass:

    ID_RECALC_GEOMETRY | ID_RECALC_COPY_ON_WRITE. This might not be so bad as there aren't so many mesh editing operations in object mode, although it might still be error prone.
  • There could be a separate kind of ID_RECALC_GEOMETRY flag for edit-mode, instead of making the current flag check the id-state. From some tests though it seems like adding similar flags ends up requiring duplicate code-paths for something that's virtually the same, so it didn't seem worthwhile.

Further Work:

  • Armatures might be able to skip a full COW copy too.
  • DEG_id_tag_update(&ob->id, ID_RECALC_GEOMETRY) on the object data still causes the mesh data to be copied while in edit-mode. Solutions to this should be investigated too.

Diff Detail

Repository
rB Blender
Branch
TEMP-D11337-UPDATE (branched from master)
Build Status
Buildable 15275
Build 15275: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.May 21 2021, 10:23 AM
Campbell Barton (campbellbarton) created this revision.
Harbormaster completed remote builds in B14681: Diff 37341.May 21 2021, 10:23 AM
Campbell Barton (campbellbarton) updated this revision to Diff 37342.May 21 2021, 10:30 AM
  • Don't use this for armatures since that should be evaluated separately.
Harbormaster completed remote builds in B14682: Diff 37342.May 21 2021, 10:30 AM
Campbell Barton (campbellbarton) retitled this revision from Remove redundant depsgraph mesh data duplication in edit-mode to Depsgraph: remove redundant mesh data duplication in edit-mode.May 21 2021, 10:39 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) added a reviewer: Sergey Sharybin (sergey).
Campbell Barton (campbellbarton) added a project: Dependency Graph.
Erick Tukuniata (erickblender) awarded a token.May 21 2021, 10:43 AM
Erick Tukuniata (erickblender) added a subscriber: Erick Tukuniata (erickblender).
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)May 21 2021, 10:53 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)May 21 2021, 10:55 AM
Sergey Sharybin (sergey) requested changes to this revision.EditedMay 21 2021, 10:56 AM
Sergey Sharybin (sergey) added a reviewer: Sybren A. Stüvel (sybren).

I do not think this is correct thing to do. There are users of the geometry data which needs datablock to be consistent: for example, animation system, possibly render engines. The geometry datablock is at very least to be ensured to be "expanded" and properties are to be synchronized into the depsgraph CoW datablock.

Edit: Please try to make the patch well-presented before assigning reviews. Not only this will ensure all the information is as clear as it possible when review starts, but also avoids spamming inboxes of the reviewers.

This revision now requires changes to proceed.May 21 2021, 10:56 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)May 21 2021, 11:10 AM
Campbell Barton (campbellbarton) added a comment.EditedMay 21 2021, 12:02 PM

I think the current patch could be made to work.
New data blocks will always get expanded, tagging that requires the COW copies are updated could use ID_RECALC_GEOMETRY | ID_RECALC_COPY_ON_WRITE.

Nevertheless, I'm not pushing for this as the final solution.

In D11337#290022, @Sergey Sharybin (sergey) wrote:

I do not think this is correct thing to do. There are users of the geometry data which needs datablock to be consistent: for example, animation system, possibly render engines. The geometry datablock is at very least to be ensured to be "expanded" and properties are to be synchronized into the depsgraph CoW datablock.

In that case it might be simpler to have a tag for shared-runtime data e.g. ID_RECALC_GEOMETRY_RUNTIME or even ID_RECALC_RUNTIME, since only the depsgraph just needs to re-evaluate the current data taking changes into account that are already in shared pointers which the COW ID's are referencing.

May be better to discuss this in chat.

Edit: Please try to make the patch well-presented before assigning reviews. Not only this will ensure all the information is as clear as it possible when review starts, but also avoids spamming inboxes of the reviewers.

Will do.

Campbell Barton (campbellbarton) planned changes to this revision.EditedMay 21 2021, 1:52 PM

Based on discussion with Sergey and Sybren in #core-module.

This patch has a problem that properties being modified currently use ID_RECALC_GEOMETRY, this patch will prevent them from updating the copy-on-write data.

Solution proposed by Sergey is to make sure non-geometry mesh properties are handled with ID_RECALC_PARAMETERS.
& mesh always expanded at least once, then this patch should work.

Germano Cavalcante (mano-wii) added a subscriber: Germano Cavalcante (mano-wii).May 21 2021, 2:10 PM
Gilberto Rodrigues (gilberto_rodrigues) awarded a token.May 22 2021, 3:14 AM
stefano severi (Tetone) awarded a token.May 24 2021, 3:53 AM
Campbell Barton (campbellbarton) mentioned this in D11377: Depsgraph: support flushing parameters without a full COW update.May 25 2021, 5:55 AM
Campbell Barton (campbellbarton) added a comment.May 25 2021, 6:46 AM

Submitted D11377: Depsgraph: support flushing parameters without a full COW update to avoid copy on write updates when changing parameters.

Campbell Barton (campbellbarton) mentioned this in T88550: Mesh Optimization Project Progress.May 25 2021, 10:32 AM
Sybren A. Stüvel (sybren) added a comment.Jun 3 2021, 12:03 PM
In D11337#290082, @Campbell Barton (campbellbarton) wrote:

This patch has a problem that properties being modified currently use ID_RECALC_GEOMETRY, this patch will prevent them from updating the copy-on-write data.

Does this mean that this patch requires D11377: Depsgraph: support flushing parameters without a full COW update to be applied first? Because that patch describes itself as "a follow up to [this patch]", so then it's a bit unclear which one "goes first". Or is the described problem simply not solved by either patch?

source/blender/depsgraph/intern/node/deg_node_component.h
170

This member function should be declared with override, so:

bool need_tag_cow_before_update() override \
172–173

Is there a specific reason why this is written as two separate if-statements, instead of simply &&-ing together the two conditions?

kursad k (kursadk) awarded a token.Jun 5 2021, 7:35 AM
Campbell Barton (campbellbarton) updated this revision to Diff 38447.Jun 17 2021, 9:13 AM
  • use 'override'
  • use single instead of nested if statements.
Harbormaster completed remote builds in B15275: Diff 38447.Jun 17 2021, 9:13 AM
Campbell Barton (campbellbarton) marked 2 inline comments as done.Jun 17 2021, 9:14 AM
Campbell Barton (campbellbarton) updated this revision to Diff 38449.Jun 17 2021, 9:18 AM
  • Add missing include
Harbormaster completed remote builds in B15277: Diff 38449.Jun 17 2021, 9:18 AM
Campbell Barton (campbellbarton) added inline comments.Jun 17 2021, 9:22 AM
source/blender/depsgraph/intern/node/deg_node_component.h
170

Shouldn't this apply to the existing DEG_COMPONENT_NODE_DECLARE_NO_COW_TAG_ON_UPDATE below? Also BoneComponentNode 's init function.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Jun 17 2021, 9:32 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Jun 17 2021, 9:36 AM
Campbell Barton (campbellbarton) added a comment.Jun 17 2021, 9:46 AM
In D11337#293638, @Sybren A. Stüvel (sybren) wrote:
In D11337#290082, @Campbell Barton (campbellbarton) wrote:

This patch has a problem that properties being modified currently use ID_RECALC_GEOMETRY, this patch will prevent them from updating the copy-on-write data.

Does this mean that this patch requires D11377: Depsgraph: support flushing parameters without a full COW update to be applied first? Because that patch describes itself as "a follow up to [this patch]", so then it's a bit unclear which one "goes first". Or is the described problem simply not solved by either patch?

The intention is to apply D11377 before this patch (updated description).

Sergey was concerned that this patch could cause problems in cases RNA parameters used a ID_RECALC_GEOMETRY tag would not work.

As far as I can tell, this never happens at the moment, nevertheless the concern is valid.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Jun 17 2021, 9:48 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Jun 17 2021, 4:33 PM
Germano Cavalcante (mano-wii) mentioned this in D11599: Depsgraph: Implement 'ID_RECALC_GEOMETRY_DEFORM'.Jun 18 2021, 3:36 PM
Germano Cavalcante (mano-wii) added a comment.Jun 18 2021, 5:20 PM

I had an idea that might be worth considering.
Based on D11599, (which "solved" the problem by creating a GEOMETRY_NO_COW component), my idea is to create a component called GEOMETRY_SHARED and an ID_RECALC_GEOMETRY_SHARED flag.
The name "SHARED" indicates that only parameters shared on different evaluated ids have been modified.
And since they're shared, there's no need to COW.

The difference between the GEOMETRY component and the GEOMETRY_SHARED component is just that the former requires COW.

Sergey Sharybin (sergey) added a comment.Jun 21 2021, 11:15 AM

Did you test non-mesh objects in edit mode/

For the D11599, it does seem that there are few different directions being investigated. Not sure if they are mutually exclusive, or solve different aspects. Can you align the forces here?

Germano Cavalcante (mano-wii) added a comment.Jun 21 2021, 1:39 PM

D11599 was postponed in favor of this one.
They are not mutually exclusive.
That one will be edited according to the decision made here.
I will be satisfied whichever path is chosen.

Campbell Barton (campbellbarton) added a comment.Jun 22 2021, 8:28 AM
In D11337#299695, @Sergey Sharybin (sergey) wrote:

Did you test non-mesh objects in edit mode/

Yes, although this patch doesn't impact other object types.

It would be good to check other object types, although duplicating most other kind of object data (armatures, curves ... etc), isn't as much of a bottleneck as it is for meshes.

For the D11599, it does seem that there are few different directions being investigated. Not sure if they are mutually exclusive, or solve different aspects. Can you align the forces here?

From checking this patch - it doesn't seem directly related to the change this makes.

Campbell Barton (campbellbarton) updated this revision to Diff 38583.Jun 22 2021, 8:32 AM

Use 'virtual bool need_tag_cow_before_update() override' based on feedback to D11377.

Harbormaster completed remote builds in B15349: Diff 38583.Jun 22 2021, 8:32 AM
Sergey Sharybin (sergey) added a comment.Jun 22 2021, 9:42 AM

Yes, although this patch doesn't impact other object types.

I don't see how this patch does not affect other object types: geometry is a generic component, and need_tag_cow_before_update doesn't do any checks either. What am I missing? Is the hypnotises that we never access object->data when in edit mode correct?
Surely, as you're saying, other object types work, which is great. I am trying to better understand what's going on.

P.S. To me the code seems fine, and does intuitive things. Just want to understand implications and be ready to possible consequences of the patch before it lands.

Campbell Barton (campbellbarton) added a comment.EditedJun 23 2021, 12:31 PM

My mistake (I was jumping between this patch and D11377 which is only for meshes).

Yes, this has been tested with all other object types that support edit-mode (linked duplicates & multi-edit).

Sergey Sharybin (sergey) accepted this revision.Jun 23 2021, 2:49 PM

Happens :)

Sergey Sharybin (sergey) removed 1 blocking reviewer(s): Sybren A. Stüvel (sybren).Jun 24 2021, 9:18 AM

Changing Sybren to non-blocking, as he wouldn't review the patch is the near-future, and I do not see anything stopping for the patch.

This revision is now accepted and ready to land.Jun 24 2021, 9:18 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Jun 24 2021, 12:06 PM
Campbell Barton (campbellbarton) mentioned this in rB51568030e9c8: Depsgraph: remove redundant mesh data duplication in edit-mode.Jun 24 2021, 12:26 PM
Campbell Barton (campbellbarton) closed this revision.Jun 24 2021, 12:26 PM

Revision Contents
Changeset List

PathSize
source/
blender/
depsgraph/
intern/
node/
23 lines

Diff 38447

View Options

source/blender/depsgraph/intern/node/deg_node_component.h

Loading...