Details

Reviewers

Sergey Sharybin (sergey)
Antonio Vazquez (antoniov)
Daniel Martinez Lara (pepeland)
Matias Mendiola (mendio)

Group Reviewers

Dependency Graph

Maniphest Tasks

T95401: GPencil: Update-on-write using an update cache

Commits

rBe2befa425a84: GPencil: Update-on-write using update cache

Summary

This implements the update cache described in T95401.

The cache is currently only used for drawing strokes, sculpting (using the push brush), and changing RNA properties on layers (e.g. changing the opacity or
the layer transform). Note: Making use of the cache throughout grease pencil will have to be done incrementally in other patches.

Demo on an Intel® Core™ i7-9700 CPU @ 3.00GHz with 8 cores:

Performance tests:
Test file: heavy_test_file.blend (from T91945)
Single GP object with:

86 layers
11k frames
300k stokes
13M points

OS: Windows 10
CPU: Intel(R) Xeon(R) E5-1650 v4 @ 3.60GHz

	Drawing strokes (average of 10)	Changing layer opacity (average of 10)	Sculpting strokes (push brush, 100 strokes)
master (2022-02-02)	1.1201844 s	1.1050539 s	0.8 - 1.1 fps
patch	0.0013894 s (~ x800)	0.0012322 s (~ x900)	20 - 24 fps

Measurements done using --debug-depsgraph-time.

Remaining issues:

Fix memory leaks

Design task: T95401
Co-authored-by: @Yann Lanthony (yann-lty)

Diff Detail

Repository: rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Antonio Vazquez (antoniov) awarded a token.Feb 1 2022, 6:01 PM

Daniel Martinez Lara (pepeland) awarded a token.Feb 1 2022, 6:16 PM

Link to the correct task

Harbormaster completed remote builds in B20280: Diff 47835.Feb 1 2022, 6:46 PM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Feb 1 2022, 6:49 PM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)

Aleš Jelovčan (frogstomp) awarded a token.Feb 1 2022, 6:56 PM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Feb 1 2022, 6:57 PM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Feb 1 2022, 7:00 PM

Antonio Vazquez (antoniov) added reviewers: Grease Pencil, Dependency Graph.Feb 1 2022, 7:01 PM

In general, I really like the idea of tagging only changed data and it's a great step to improve the handling of very large files that are normally used in production.

You both have done a great job!

source/blender/blenkernel/BKE_gpencil_update_cache.h
138	Not sure if this is the best naming. `Struct` has some `C` naming implications and maybe would be better to use something like `BKE_gpencil_tag_data_members_update` or `BKE_gpencil_tag_only__members_update`. Not sure about the name, we need find a better one.
source/blender/blenkernel/intern/gpencil.c
993	As this function copy data members, it's very easy to miss here something when is added to DNA struct, so it would be good to put some comments in the DNA definition about that this function needs to be update also.

Antonio Vazquez (antoniov) added inline comments.Feb 1 2022, 7:10 PM

source/blender/blenkernel/BKE_gpencil_update_cache.h
47	Same comment about naming... if we change the BKE function name, we must align this tag name too.

Garek (Garek) awarded a token.Feb 1 2022, 7:17 PM

Garek (Garek) added a subscriber: Garek (Garek).

Gavriel Lavi (Gavriel5578) awarded a token.Feb 2 2022, 12:18 AM

Rename struct_copy to light_copy
Simplify cache_node_update and add comments
Add comments to DNA for BKE_*_copy_settings functions
Add light updates to layers in the rna update

Co-authored-by: @Yann Lanthony (yann-lty)

Harbormaster completed remote builds in B20292: Diff 47859.Feb 2 2022, 11:50 AM

Falk David (filedescriptor) removed a subscriber: Yann Vernier (yann).Feb 2 2022, 11:52 AM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Feb 2 2022, 12:01 PM

Fix memory leak for full updates on strokes
Merge branch 'master'

Harbormaster completed remote builds in B20293: Diff 47862.Feb 2 2022, 12:43 PM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Feb 2 2022, 12:44 PM

Falk David (filedescriptor) marked 3 inline comments as done.

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Feb 2 2022, 1:24 PM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)

Falk David (filedescriptor) updated this revision to Diff 47873.Feb 2 2022, 7:04 PM

This comment was removed by Falk David (filedescriptor).

Harbormaster completed remote builds in B20301: Diff 47873.Feb 2 2022, 7:04 PM

Undo wrong commits

Harbormaster completed remote builds in B20302: Diff 47874.Feb 2 2022, 7:07 PM

Falk David (filedescriptor) mentioned this in D13996: GPencil: Undo System.Feb 2 2022, 7:20 PM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Feb 3 2022, 10:15 AM

Merge branch 'master'

Harbormaster completed remote builds in B20320: Diff 47905.Feb 3 2022, 12:32 PM

Helge Maus (pixeltrain) awarded a token.Feb 3 2022, 2:51 PM

johan tri handoyo (johantri) awarded a token.Feb 4 2022, 9:37 AM

This is mostly all internal to grease pencil I don't have much understanding or comments on that. Just a note about the depsgraph integration.

source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc
906–928	I think it would be better to reorganize this code to make it more similar to other data types. For the `!!BKE_gpencil_check_copy_on_write_needed` case, move it before `RuntimeBackup` like the edit mode exception. Maybe it also needs that same `is_cow_explicitly_tagged` check? Something like: if (check_datablock_expanded(id_cow) && !id_node->is_cow_explicitly_tagged) { const ID_Type id_type = GS(id_orig->name); if (OB_DATA_SUPPORT_EDITMODE(id_type) && BKE_object_data_is_in_editmode(id_orig)) { ... } else if {id_type == ID_GD && BKE_gpencil_check_copy_on_write_needed) { ... } Then for the other case, I think a `RuntimeBackup` should be implemented for grease pencil consistent with other datablocks.

Merge branch 'master'
Add RuntimeBackup for grease pencil
Ensure update-on-write only in active depsgraph

Harbormaster completed remote builds in B20349: Diff 47982.Feb 4 2022, 6:32 PM

Falk David (filedescriptor) marked an inline comment as done.Feb 4 2022, 6:32 PM

Remove tagging copy-on-write tag on gpd id
Merge branch 'master' into patch/gpencil-update-on-write

Harbormaster completed remote builds in B20380: Diff 48038.Feb 7 2022, 3:47 PM

For the depsgraph side I have some east-to-address inlined comments. Mainly need to ensure that depsgraph developers understand why things are done the way they are done.

The cache system as a whole is a bit harder for me to justify from the "is there a simpler way of dealing with performance". The design does sound reasonable, so kinda would trust the gpencil team to justify that it is a necessary "complication" to go to.
It also seems something "internal" in a sense that if there are issues we can re-iterate over it without loosing file compatibility. So unless someone else want to drop by and lead a project I'd say it is up to the gpencil team to design and implement the caching needed for painting.

The only thing is that ideally we'd need to only use the fast behavior and remove the use_gpencil_update_cache setting.

source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc
895	Don't use `else`after `return`. The `BKE_gpencil_update_on_write_check` sounds ambiguous. Maybe something `BKE_gpencil_check_can_avoid_full_copy_on_write()` ?
source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_gpencil.cc
17	Copyright year in new files.
46–49	Some explanation is needed about why the cache is to be freed here, and why only for active depsgraph. Something to help future depsgraph developers to understand what's and why something is going on.

Jon Henry M. Faltis 9077149 (295032) awarded a token.Feb 8 2022, 6:19 AM

Merge with master
Rename kernel function
Add comments

Harbormaster completed remote builds in B20402: Diff 48077.Feb 8 2022, 11:02 AM

Falk David (filedescriptor) marked 2 inline comments as done.Feb 8 2022, 11:04 AM

In D13984#373753, @Sergey Sharybin (sergey) wrote:

The only thing is that ideally we'd need to only use the fast behavior and remove the use_gpencil_update_cache setting.

Yes, if we are going to remove the debug option, then we should fully move to the update cache (which should probably be done in some other patch).

Thanks for the update! From the depsgraph side it seems good now.

For the gpencil part best would be to get review from the gpencil module members.

Matias Mendiola (mendio) awarded a token.Feb 8 2022, 3:23 PM

Daniel Martinez Lara (pepeland) accepted this revision.Feb 8 2022, 4:26 PM

LGTM, but keep pending because I want a double check of the patch by @Daniel Martinez Lara (pepeland) and @Matias Mendiola (mendio) to be sure the patch is working as expected.

Matias Mendiola (mendio) accepted this revision.Feb 8 2022, 5:11 PM

The patch is ready to be moved to master. Thanks to both of you for your work and for such an important modification that solves one of the main problems that existed when working with heavy files. Congratulations!!

This revision is now accepted and ready to land.Feb 8 2022, 5:15 PM

There are a few things that we feel like need to be done before committing.

The BLI_findlinkfrom function that was added should be added to the tests in source/blender/blenlib/tests/BLI_listbase_test.cc.
The light tag in rna_GPencil_update should be removed for now as this was only part of our testing. It can be added after the merge in another commit.
1. Note: The problem is that in rna_property_update there is an explicit tag DEG_id_tag_update(ptr->owner_id, ID_RECALC_COPY_ON_WRITE); which will skip the update cache.
The debug boolean should be removed and the update cache should always be enabled. But it will only be used when the tagging functions are properly called and otherwise fallback to a copy-on-write.