Fix #104798: Slow frame-change & scrubbing with many objects #104801

Campbell Barton · 2023-02-16T02:08:48+01:00

Campbell Barton commented

2023-02-16 02:08:48 +01:00

Avoid re-copying the scene when the scene is tagged for frame change.

For scenes with many objects scrubbing & jumping between first/last caused noticeably worse performances.

Resolve by:

Returning false from need_tag_cow_before_update when the recalc flag is ID_RECALC_FRAME_CHANGE.
Change deg_graph_tag_parameters_if_needed to consider ID_RECALC_FRAME_CHANGE a clean flag.

Avoid re-copying the scene when the scene is tagged for frame change. For scenes with many objects scrubbing & jumping between first/last caused noticeably worse performances. Resolve by: - Returning false from need_tag_cow_before_update when the recalc flag is ID_RECALC_FRAME_CHANGE. - Change deg_graph_tag_parameters_if_needed to consider ID_RECALC_FRAME_CHANGE a clean flag.

👍 1

Campbell Barton commented

2023-02-16 02:25:16 +01:00

@blender-bot build

Campbell Barton commented

2023-02-16 03:14:50 +01:00

@blender-bot build linux

Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 23857dc29c to 3e4349f036

2023-02-16 04:01:59 +01:00

Compare

Campbell Barton requested review from Sybren A. Stüvel 2023-02-16 04:07:53 +01:00

Campbell Barton requested review from Sergey Sharybin 2023-02-16 04:07:54 +01:00

Campbell Barton requested review from Bastien Montagne 2023-02-16 04:07:54 +01:00

Campbell Barton referenced this pull request

2023-02-16 05:08:24 +01:00

Use hash for Collection.gobject lookup, speedup object linking #104553

Sergey Sharybin commented

2023-02-16 10:38:11 +01:00

All the node tress regardless of whether they are "embedded" into a "parent" datablock are treated as a "stand-alone" ID data-blocks in the dependency graph builder. This takes care of ensuring that:

The "embedded" node tree itself have its dedicated evaluated state.
The "parent" data-block the node tree belongs to have the field re-mapped to the evaluated state.

The goal of the special cases for the scene builder was to avoid special cases in the dependency graph nodes and relations builders, at a cost of special tricks of during copy-on-write and during tagging.

The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original master_collection, which is violation of the original/evaluated state design.

A proper solution would be to make it so that NodeType::AUDIO does not involve the copy-on-write tag for the D_RECALC_FRAME_CHANGE tag.

The most straightforward way it seems to be to make it so IDRecalcFlag is passed to need_tag_cow_before_update, and have an implementation of AudioComponentNode which will return false if the tag == ID_RECALC_FRAME_CHANGE.

All the node tress regardless of whether they are "embedded" into a "parent" datablock are treated as a "stand-alone" ID data-blocks in the dependency graph builder. This takes care of ensuring that: 1. The "embedded" node tree itself have its dedicated evaluated state. 2. The "parent" data-block the node tree belongs to have the field re-mapped to the evaluated state. The goal of the special cases for the scene builder was to avoid special cases in the dependency graph nodes and relations builders, at a cost of special tricks of during copy-on-write and during tagging. The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original `master_collection`, which is violation of the original/evaluated state design. --- A proper solution would be to make it so that `NodeType::AUDIO` does not involve the copy-on-write tag for the `D_RECALC_FRAME_CHANGE` tag. The most straightforward way it seems to be to make it so `IDRecalcFlag` is passed to `need_tag_cow_before_update`, and have an implementation of `AudioComponentNode` which will return `false` if the `tag == ID_RECALC_FRAME_CHANGE`.

Sergey Sharybin requested changes 2023-02-16 10:38:29 +01:00

Dismissed

Sergey Sharybin left a comment

Marking as requested changes.

Campbell Barton commented

2023-02-17 03:37:17 +01:00

The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original master_collection, which is violation of the original/evaluated state design.

I'm aware of this and checked to make sure this wasn't happening, attached a patch that adds a print to deg_expand_copy_on_write_datablock, in my tests it's never sharing the pointer. Is there another point in the code where the pointers would be shared?

> The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original `master_collection`, which is violation of the original/evaluated state design. I'm aware of this and checked to make sure this wasn't happening, attached a patch that adds a print to `deg_expand_copy_on_write_datablock`, in my tests it's never sharing the pointer. Is there another point in the code where the pointers would be shared?

deg_master_collection_printf.diff

951 B

Campbell Barton requested review from Sergey Sharybin 2023-02-17 03:45:12 +01:00

Brecht Van Lommel added this to the Core project 2023-02-20 10:40:36 +01:00

Sybren A. Stüvel commented

2023-02-20 16:45:03 +01:00

I'm aware of this and checked to make sure this wasn't happening

If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader.

Also I don't think that adding a printf() somewhere and not seeing it print anything during testing is enough of a motivation to move away from an established design.

> I'm aware of this and checked to make sure this wasn't happening If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader. Also I don't think that adding a `printf()` somewhere and not seeing it print anything during testing is enough of a motivation to move away from an established design.

Sergey Sharybin requested changes 2023-02-20 16:58:20 +01:00

Dismissed

Sergey Sharybin left a comment

Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to.

I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design.

The only reason why the evaluated ID for the master_collection is created which I can think of is that the built-in into scene master_collection is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the masetr_collection the same way as the nodetree. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling master_collection and not the speedup.

For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all).

Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to. I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design. The only reason why the evaluated ID for the `master_collection` is created which I can think of is that the built-in into scene `master_collection` is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the `masetr_collection` the same way as the `nodetree`. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling `master_collection` and not the speedup. For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all).

Campbell Barton commented

2023-02-21 10:29:05 +01:00

I'm aware of this and checked to make sure this wasn't happening

If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader.

As far as I can see it's not violating any existing design (as in I checked to make sure it's not sharing pointers).

As I understand it - the master_collection is an embedded ID, copying the COW scene already copies the ID, so it makes sense to treat this the same as other embedded ID's.

> > I'm aware of this and checked to make sure this wasn't happening > > If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader. As far as I can see it's not violating any existing design (as in I checked to make sure it's not sharing pointers). As I understand it - the `master_collection` is an embedded ID, copying the COW scene already copies the ID, so it makes sense to treat this the same as other embedded ID's.

Campbell Barton commented

2023-02-21 10:57:37 +01:00

Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to.

My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks.

I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design.

Fair enough, however duplicating scene (the COW copy) always duplicates the master_collection so as far as I can see this will never share a pointer with the original data.

The only reason why the evaluated ID for the master_collection is created which I can think of is that the built-in into scene master_collection is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the masetr_collection the same way as the nodetree. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling master_collection and not the speedup.

The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in G_MAIN->collections for e.g.)

For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all).

Which area are you referring to?
This patch updates ntree_hack_remap_pointers & scene_foreach_id uses BKE_library_foreach_ID_embedded for scene->master_collection.

> Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to. My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks. > I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design. Fair enough, however duplicating scene (the COW copy) always duplicates the `master_collection` so as far as I can see this will never share a pointer with the original data. > The only reason why the evaluated ID for the `master_collection` is created which I can think of is that the built-in into scene `master_collection` is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the `masetr_collection` the same way as the `nodetree`. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling `master_collection` and not the speedup. The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in `G_MAIN->collections` for e.g.) > For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all). Which area are you referring to? This patch updates `ntree_hack_remap_pointers` & `scene_foreach_id` uses `BKE_library_foreach_ID_embedded` for `scene->master_collection`.

Campbell Barton requested review from Sergey Sharybin 2023-02-27 12:08:10 +01:00

Sergey Sharybin commented

2023-02-28 11:23:56 +01:00

My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks.

Yeah, something that i guess could have been described better in the comment above.

Basically, the dependency graph handles the "embedded" IDs the same way as non-embedded onces. This means, for example, that node tree relation builder does not destinguish non-embedded node groups from embedded compositor/shader. And the copy-on-write takes care of remapping the "embedded" pointers to the, what appears to be non-embedded from the depsgraph perspective, ID.

In practice this means that the graph builder and the copy-on-write update step needs to make the same (weak) assumptions.

Fair enough, however duplicating scene (the COW copy) always duplicates the master_collection so as far as I can see this will never share a pointer with the original data.

I don't really understand how this works.

First of all, in the case of embedded node trees we do not want the ID copy function to duplicate or allocate that pointer. The depsgraph takes care of assigning it to a proper evaluated state.

The code in the patch temporarily assigns the master_collection to nullptr, and then calls the ID copy function, and restores the pointer back. If the ID copy function allocates master_collection then there is a memory leak. But in any case, the master_collection is restored to its original value.

In the case of embedded node trees it is the BKE_library_foreach_ID_link() + foreach_libblock_remap_callback() which takes care of remapping the embedded pointer to an evaluated node ID pointer from the depsgraph.

If the master_collection follows the same exact code as the embedded node trees and it has different pointer in the evaluated scene it means that the master collection somehow got created in the dependency graph.

Thing is: unlike the embedded node trees for scene compositor and material shader the dependency graph builder does not access scene->master_collection at all. So, if there is no such explicit access where does the ID node for the master_collection is coming from?

Which leads to the next point...

The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in G_MAIN->collections for e.g.)

So I tohught as well.. However. Without going deep into code or trying to find an actual design, lets use a trusty old printf way of checking the obvious suspect: view_layer->layer_collections.

diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
index d7420b91db4..5fc316563a2 100644
--- a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
+++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
@@ -53,6 +53,7 @@ void DepsgraphNodeBuilder::build_layer_collections(ListBase *lb)
     }
     if ((lc->flag & LAYER_COLLECTION_EXCLUDE) == 0) {
       build_collection(lc, lc->collection);
+      printf("%d\n", lc->collection == scene_->master_collection);
     }
     build_layer_collections(&lc->layer_collections);
   }

Sure enough, the output is:

sergey@ws-cheese ~/Developer/blender/build/release % ./bin/Blender.app/Contents/MacOS/Blender --factory-startup
Device with name AMD Radeon PRO W6800X supports metal minimum requirements
METAL API - DETECTED GPU: AMD Radeon PRO W6800X
1
0

So is it by design? Is it always guaranteed that the first layer collection references embedded master_collection?

Which area are you referring to?
This patch updates ntree_hack_remap_pointers & scene_foreach_id uses ?BKE_library_foreach_ID_embedded for scene->master_collection.

The entire copy-on-write can/should be skipped on the frame change. Dependency graph's DEG_evaluate_on_framechange takes care of updating the frame value. Doing this Scene datablock free + duplicate + BKE_library_foreach_ID_embedded is not needed at all.

It could as simple as making it so Audio component returns false from its need_tag_cow_before_update, at least on a frame change. For volume changes we still might want to copy-on-write to grab the new value of volume.

This sounds more logical/correct behavior of frame change handling, and it solves the root of the problem stated in the description: the ID_RECALC_FRAME_CHANGE causing heavy scene copy-on-write.

Long story short: if copy-on-write is not needed, don't do it at all.

> My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks. Yeah, something that i guess could have been described better in the comment above. Basically, the dependency graph handles the "embedded" IDs the same way as non-embedded onces. This means, for example, that node tree relation builder does not destinguish non-embedded node groups from embedded compositor/shader. And the copy-on-write takes care of remapping the "embedded" pointers to the, what appears to be non-embedded from the depsgraph perspective, ID. In practice this means that the graph builder and the copy-on-write update step needs to make the same (weak) assumptions. > Fair enough, however duplicating scene (the COW copy) always duplicates the master_collection so as far as I can see this will never share a pointer with the original data. I don't really understand how this works. First of all, in the case of embedded node trees we do not want the ID copy function to duplicate or allocate that pointer. The depsgraph takes care of assigning it to a proper evaluated state. The code in the patch temporarily assigns the `master_collection` to `nullptr`, and then calls the ID copy function, and restores the pointer back. If the ID copy function allocates `master_collection` then there is a memory leak. But in any case, the `master_collection` is restored to its original value. In the case of embedded node trees it is the `BKE_library_foreach_ID_link()` + `foreach_libblock_remap_callback()` which takes care of remapping the embedded pointer to an evaluated node ID pointer from the depsgraph. If the `master_collection` follows the same exact code as the embedded node trees and it has different pointer in the evaluated scene it means that the master collection somehow got created in the dependency graph. Thing is: unlike the embedded node trees for scene compositor and material shader the dependency graph builder does not access `scene->master_collection` at all. So, if there is no such explicit access where does the ID node for the `master_collection` is coming from? Which leads to the next point... > The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in G_MAIN->collections for e.g.) So I tohught as well.. However. Without going deep into code or trying to find an actual design, lets use a trusty old `printf` way of checking the obvious suspect: `view_layer->layer_collections`. ``` diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc index d7420b91db4..5fc316563a2 100644 --- a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc +++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc @@ -53,6 +53,7 @@ void DepsgraphNodeBuilder::build_layer_collections(ListBase *lb) } if ((lc->flag & LAYER_COLLECTION_EXCLUDE) == 0) { build_collection(lc, lc->collection); + printf("%d\n", lc->collection == scene_->master_collection); } build_layer_collections(&lc->layer_collections); } ``` Sure enough, the output is: ``` sergey@ws-cheese ~/Developer/blender/build/release % ./bin/Blender.app/Contents/MacOS/Blender --factory-startup Device with name AMD Radeon PRO W6800X supports metal minimum requirements METAL API - DETECTED GPU: AMD Radeon PRO W6800X 1 0 ``` So is it by design? Is it always guaranteed that the first layer collection references embedded `master_collection`? > Which area are you referring to? > This patch updates ntree_hack_remap_pointers & scene_foreach_id uses ?BKE_library_foreach_ID_embedded for scene->master_collection. The entire copy-on-write can/should be skipped on the frame change. Dependency graph's `DEG_evaluate_on_framechange` takes care of updating the frame value. Doing this Scene datablock free + duplicate + `BKE_library_foreach_ID_embedded` is not needed at all. It could as simple as making it so `Audio` component returns `false` from its `need_tag_cow_before_update`, at least on a frame change. For volume changes we still might want to copy-on-write to grab the new value of volume. This sounds more logical/correct behavior of frame change handling, and it solves the root of the problem stated in the description: the `ID_RECALC_FRAME_CHANGE` causing heavy scene copy-on-write. Long story short: if copy-on-write is not needed, don't do it at all.

Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 3e4349f036 to f9ddbce145

2023-03-02 07:12:49 +01:00

Compare

Campbell Barton commented

2023-03-02 07:14:06 +01:00

Confirmed that skipping the COW tag for Audio components fixes the issue on frame change, so if COW isn't needed for anything else - this fixes #104798.

Skipping the collection copy still seems worthwhile though, practically all scene settings (some material shading settings) are causing the collection to be duplicated (so changing the render-resolution or scene samples duplicates the collection and rebuilds it's hash on each redraw - which can cause a noticeable slowdown).

I wasn't aware the master_collection can be referenced from LayerCollections. As long as the LayerCollection is part of the scene - I think this could be supported without causing problem however that needs to be investigated in more detail.

Updated the patch to change Audio tagging not to copy-on-write as suggested.

@blender-bot build linux

Confirmed that skipping the COW tag for Audio components fixes the issue on frame change, so if COW isn't needed for anything else - this fixes #104798. Skipping the collection copy still seems worthwhile though, practically all scene settings (some material shading settings) are causing the collection to be duplicated (so changing the render-resolution or scene samples duplicates the collection and rebuilds it's hash on each redraw - which can cause a noticeable slowdown). I wasn't aware the `master_collection` can be referenced from `LayerCollections`. As long as the `LayerCollection` is part of the scene - I think this could be supported without causing problem however that needs to be investigated in more detail. ---- Updated the patch to change Audio tagging not to copy-on-write as suggested. ---- @blender-bot build linux

Sergey Sharybin commented

2023-03-07 09:45:37 +01:00

The copy-on-write is only obviously not needed for the ID_RECALC_FRAME_CHANGE, as the dependency graph is taking care of the frame change. For other types the dependency graph does not ensure anything, so other tags might still need copy-on-write to happen. See my comment above where Iv'e mentioned this.

For example ID_RECALC_AUDIO_MUTE and ID_RECALC_AUDIO_LISTENER definitely need copy-on-write.

The ID_RECALC_AUDIO_VOLUME and ID_RECALC_AUDIO_FPS are a bit more tricky. If those flags will not imply copy-on-write nothing will immediately break. But! This is only because of they're used in combination with ID_RECALC_SEQUENCER_STRIPS. Arguably, this combination is wrong and the tag should only happen for VOLUME and FPS, and then the tag is to be flushed to the sequencer components. In any case, designing component copy-on-write component based on the current state of flags used to tag is weak, and we should not do so.

The copy-on-write is only obviously not needed for the `ID_RECALC_FRAME_CHANGE`, as the dependency graph is taking care of the frame change. For other types the dependency graph does not ensure anything, so other tags might still need copy-on-write to happen. See my comment above where Iv'e mentioned this. For example `ID_RECALC_AUDIO_MUTE` and `ID_RECALC_AUDIO_LISTENER` definitely need copy-on-write. The `ID_RECALC_AUDIO_VOLUME` and `ID_RECALC_AUDIO_FPS` are a bit more tricky. If those flags will not imply copy-on-write nothing will immediately break. But! This is only because of they're used in combination with `ID_RECALC_SEQUENCER_STRIPS`. Arguably, this combination is wrong and the tag should only happen for VOLUME and FPS, and then the tag is to be flushed to the sequencer components. In any case, designing component copy-on-write component based on the current state of flags used to tag is weak, and we should not do so.

Sybren A. Stüvel commented

2023-11-07 16:06:03 +01:00

For the record, it seems that Sergey had some remarks that haven't resulted in a change of this PR yet. I was actually waiting for that to happen before diving into the code myself.

Bastien Montagne requested changes 2024-02-12 10:38:49 +01:00

Bastien Montagne left a comment

Is this still relevant PR?

Campbell Barton force-pushed pr-scene-collection-skip-on-cow from f9ddbce145 to 0a4907dae6

2024-02-14 07:50:50 +01:00

Compare

Campbell Barton commented

2024-02-14 08:00:45 +01:00

The issue described in #104798 still exists.

Updated the PR & description with the change Sergey suggested (passing a recalc flag to AudioComponentNode::need_tag_cow_before_update).

This didn't fix the problem on it's own though, deg_graph_tag_parameters_if_needed also needed to exclude ID_RECALC_FRAME_CHANGE from clean_flags, which is correct as far as I can see since the depsgraph writes into the COW scenes current frame directly.

The issue described in #104798 still exists. Updated the PR & description with the change Sergey suggested (passing a recalc flag to `AudioComponentNode::need_tag_cow_before_update`). This didn't fix the problem on it's own though, `deg_graph_tag_parameters_if_needed` also needed to exclude `ID_RECALC_FRAME_CHANGE` from `clean_flags`, which is correct as far as I can see since the depsgraph writes into the COW scenes current frame directly.

Campbell Barton requested review from Bastien Montagne 2024-02-14 08:01:13 +01:00

Campbell Barton commented

2024-02-14 08:01:46 +01:00

@blender-bot build

Sybren A. Stüvel reviewed 2024-02-20 11:33:21 +01:00

Sybren A. Stüvel left a comment

Apart from a few small remarks (see inline notes) the code LGTM.

I'm having a harder time correlating the results to the behaviour described in #104798, though.

	Without patch	With patch
`USE_SCENE_COLLECTION = False`	~24 FPS	~57 FPS
`USE_SCENE_COLLECTION = True`	~23 FPS	~56 FPS

So yes, this patch seems to increase performance quite considerably 👍
I can't seem to reproduce the issue in the bug report, though, as for me both 'scene collection' and 'other collection' cases are equally fast.

I tested this PR against 8a4c45a202, which seems to be the parent commit when I pull this PR.

Update: the script to reproduce the issue from #104798 doesn't include audio, though, so I also don't feel that it is sufficient to test the changes in this PR. @ideasman42 how did you test? Can we reproduce that?

Apart from a few small remarks (see inline notes) the code LGTM. I'm having a harder time correlating the results to the behaviour described in #104798, though. | | Without patch | With patch | | -- | -- | -- | | `USE_SCENE_COLLECTION = False` | ~24 FPS | ~57 FPS | | `USE_SCENE_COLLECTION = True` | ~23 FPS | ~56 FPS | So yes, this patch seems to increase performance quite considerably :+1: I can't seem to reproduce the issue in the bug report, though, as for me both 'scene collection' and 'other collection' cases are equally fast. I tested this PR against 8a4c45a2025906e02268dbcd073c7d47ddfce20d, which seems to be the parent commit when I pull this PR. **Update:** the script to reproduce the issue from #104798 doesn't include audio, though, so I also don't feel that it is sufficient to test the changes in this PR. @ideasman42 how did you test? Can we reproduce that?

source/blender/depsgraph/intern/node/deg_node_component.hh Outdated

						
				@ -241,1 +240,4 @@

				/* Audio component. */

				struct AudioComponentNode : public ComponentNode {

				  virtual bool need_tag_cow_before_update(IDRecalcFlag tag) override

Sybren A. Stüvel commented

2024-02-20 11:06:38 +01:00

const IDRecalcFlag flag

`const IDRecalcFlag flag`

ideasman42 marked this conversation as resolved

source/blender/depsgraph/intern/node/deg_node_component.hh Outdated

						
				@ -242,0 +244,4 @@

				  {

				    /* Frame change doesn't require a copy of the scene, doing so can be a heavy operation

				     * especially when the collection contains many objects, see #104798. */

				    if (tag == ID_RECALC_FRAME_CHANGE) {

Sybren A. Stüvel commented

2024-02-20 11:00:04 +01:00

Personally I'd prefer return tag != ID_RECALC_FRAME_CHANGE;

No strong feelings, it mostly depends on how likely it is that other cases will be added. If you keep the if, I think it's better to put the explanation inside the conditional block.

Personally I'd prefer `return tag != ID_RECALC_FRAME_CHANGE;` No strong feelings, it mostly depends on how likely it is that other cases will be added. If you keep the `if`, I think it's better to put the explanation inside the conditional block.

ideasman42 marked this conversation as resolved

Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 0a4907dae6 to 8541e4808e

2024-02-20 12:54:42 +01:00

Compare

Campbell Barton referenced this pull request

2024-02-20 13:19:14 +01:00

Slower frame-change with many objects in the scenes collection #104798

Campbell Barton commented

2024-02-20 13:33:17 +01:00

Interesting, I wasn't aware of this performance difference, when updating the patch I was just checking the slow case was faster, but the difference in performance doesn't depend much on USE_SCENE_COLLECTION anymore and there is a speedup in both cases on my system too.

Increasing performance for both cases, changed between 3.6 & 4.0 (where there is a big difference in performance in 3.6 but not in 4.0 and newer), noted in #104798.

@dr.sybren audio can be tested by manually adding it to the sequencer, scrubbing with the cursor & arrow keys works as before. Animated audio settings could depend on having a scene copy although this seems to be broken (animating the scenes volume never updates & looks to be set at the volume when playback starts, even the SOUND_OT_update_animation_flags operator doesn't seem to be working).

Interesting, I wasn't aware of this performance difference, when updating the patch I was just checking the slow case was faster, but the difference in performance doesn't depend much on `USE_SCENE_COLLECTION` anymore and there is a speedup in both cases on my system too. Increasing performance for both cases, changed between 3.6 & 4.0 (where there is a big difference in performance in 3.6 but not in 4.0 and newer), noted in #104798. @dr.sybren audio can be tested by manually adding it to the sequencer, scrubbing with the cursor & arrow keys works as before. Animated audio settings could depend on having a scene copy although this seems to be broken (animating the scenes volume never updates & looks to be set at the volume when playback starts, even the `SOUND_OT_update_animation_flags` operator doesn't seem to be working).

Campbell Barton changed title from ~~Fix #104798: Slower frame-change with many objects in the scenes collection~~ to Fix #104798: Slow frame-change & scrubbing with many objects

2024-02-21 07:30:25 +01:00

Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 8541e4808e to d3490494bb

2024-04-05 08:38:46 +02:00

Compare

Campbell Barton commented

2024-04-12 15:59:33 +02:00

@blender-bot build

Sergey Sharybin commented

2024-04-16 18:01:20 +02:00

@ideasman42 On the code side I think it is implementing all the things we've discussed before, and that part seems fine now. I run tests with some of anim files we've got, and all of them seem to work correctly.

I did not see performance impact on the production scenes, but with the setup from the original report (created by a script) there is quite measurable boost in FPS. And logically AUDIO should be causing copy-for-eval on frame change.

The thing which would be nice to clarify is this:

Animated audio settings could depend on having a scene copy although this seems to be broken

Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed?

@ideasman42 On the code side I think it is implementing all the things we've discussed before, and that part seems fine now. I run tests with some of anim files we've got, and all of them seem to work correctly. I did not see performance impact on the production scenes, but with the setup from the original report (created by a script) there is quite measurable boost in FPS. And logically AUDIO should be causing copy-for-eval on frame change. The thing which would be nice to clarify is this: > Animated audio settings could depend on having a scene copy although this seems to be broken Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed?

Campbell Barton commented

2024-04-17 03:13:28 +02:00

Animated audio settings could depend on having a scene copy although this seems to be broken

Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed?

It's already broken before this PR, reported the bug #120726.

It looks like SOUND_OT_update_animation_flags isn't called anywhere, yet the flags it sets are expected to be used for animated audio to work properly.

> > Animated audio settings could depend on having a scene copy although this seems to be broken > > Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed? It's already broken before this PR, reported the bug #120726. It looks like `SOUND_OT_update_animation_flags` isn't called anywhere, yet the flags it sets are expected to be used for animated audio to work properly.

Sergey Sharybin approved these changes 2024-04-17 10:50:39 +02:00

Sergey Sharybin left a comment

Thanks for clarification. I don't think that issue should be holding off this PR.

👍 1 🎉 1

Campbell Barton referenced this issue from a commit

2024-04-18 05:51:51 +02:00

Fix #104798: Slow frame-change with objects in the scenes collection

Campbell Barton manually merged commit 7047715c6b into main

2024-04-18 05:52:21 +02:00

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

Fix #104798: Slow frame-change & scrubbing with many objects #104801