blender
/
blender
129
398
Fork 572
Code Issues 6.2k Pull Requests 748 Projects 19 Wiki Activity

Undo: support implicit-sharing in memfile undo step #106903

Merged
Jacques Lucke merged 78 commits from JacquesLucke/blender:implicit-sharing-undo into main 2024-02-29 17:15:09 +01:00
Conversation 16 Commits 78 Files Changed 9 +163 -15
Jacques Lucke commented 2023-04-13 15:08:20 +02:00
Member
Copy Link

This adds implicit sharing support for the MemFile undo-step. This decreases memory use and increases performance.

Implicit sharing allows the undo system to take (shared) ownership of some data. Previously, the data would always be serialized and compared to the previous undo-step. So this turns an O(n) operation into O(1) (in terms of memory usage and time).

Read/write code that wants to make use of this has to use the new BLO_read_shared and BLO_write_shared functions respectively. Those either make use of implicit-sharing internally or do the "full" read/write based on a passed-in function. It seems possible to use the same API in the future to store shared data to .blend files.

Improvements:

  • Much faster undo step creation in many cases by avoiding the majority data copies and equality checks. This fixes #98574. I found undo step creation and undo step decoding to be 2-5 times faster in some demo files from the blender website and in some production files from the Heist project.
  • Reduced memory usage when there is large data in bmain. For example, when loading the same highly subdivided mesh that I used in #106228 the memory usage is 1.03 GB now (compared to 1.62 GB in main currently). The main remaining copy of the data now is done by rendering code.
  • Some significant performance improvements were also measured for the new grease pencil type (#105540).

There is one main downside of using implicit-sharing as implemented here: MemFile undo steps can't be written as .blend files anymore. This has a few consequences:

  • Auto-save becomes slower (up to 3x), because it can't just save the previous undo step anymore and does a normal save instead. This has been discussed in more detail here. It would be nice to work towards asynchronous auto-save to alleviate this problem. Some previous work has been done to reduce the impact of this change: 41b10424c7 and f0f304e240.
  • Writing quit.blend has to do a normal file save now. So it's a bit slower too, but it's less of a problem in practice.
  • The USE_WRITE_CRASH_BLEND functionality does not work anymore. It doesn't seem to be used by anyone.

There are also benefits to not writing MemFile from undo steps to disk. It allows us to more safely do undo-specific optimizations without risking corrupted .blend files. This is especially useful when we want to preserve forward compatibility in some cases. This requires converting data before writing the .blend files, but this conversion is not necessary for undo steps. Trying to implement this kind of optimization in the past has often lead to bugs (e.g. 43b37fbc93).

Another new problem is that it is harder to know the size of each undo step. Currently, a heuristic is used to approximate the memory usage, but better solutions could be found if necessary.

This adds implicit sharing support for the `MemFile` undo-step. This decreases memory use and increases performance. Implicit sharing allows the undo system to take (shared) ownership of some data. Previously, the data would always be serialized and compared to the previous undo-step. So this turns an O(n) operation into O(1) (in terms of memory usage and time). Read/write code that wants to make use of this has to use the new `BLO_read_shared` and `BLO_write_shared` functions respectively. Those either make use of implicit-sharing internally or do the "full" read/write based on a passed-in function. It seems possible to use the same API in the future to store shared data to .blend files. Improvements: * Much faster undo step creation in many cases by avoiding the majority data copies and equality checks. This fixes #98574. I found undo step creation and undo step decoding to be 2-5 times faster in some demo files from the blender website and in some production files from the Heist project. * Reduced memory usage when there is large data in `bmain`. For example, when loading the same highly subdivided mesh that I used in #106228 the memory usage is 1.03 GB now (compared to 1.62 GB in `main` currently). The main remaining copy of the data now is done by rendering code. * Some significant performance improvements were also measured for the new grease pencil type (#105540). There is one main downside of using implicit-sharing as implemented here: `MemFile` undo steps can't be written as .blend files anymore. This has a few consequences: * Auto-save becomes slower (up to 3x), because it can't just save the previous undo step anymore and does a normal save instead. This has been discussed in more detail [here](https://devtalk.blender.org/t/remove-support-for-saving-memfile-undo-steps-as-blend-files-proposal/33544). It would be nice to work towards asynchronous auto-save to alleviate this problem. Some previous work has been done to reduce the impact of this change: 41b10424c7e01063e769bfb57521559b270c82e6 and f0f304e24079827c07aee9306ea6041daba57d3a. * Writing `quit.blend` has to do a normal file save now. So it's a bit slower too, but it's less of a problem in practice. * The `USE_WRITE_CRASH_BLEND` functionality does not work anymore. It doesn't seem to be used by anyone. There are also benefits to not writing `MemFile` from undo steps to disk. It allows us to more safely do undo-specific optimizations without risking corrupted .blend files. This is especially useful when we want to preserve forward compatibility in some cases. This requires converting data before writing the .blend files, but this conversion is not necessary for undo steps. Trying to implement this kind of optimization in the past has often lead to bugs (e.g. 43b37fbc93). Another new problem is that it is harder to know the size of each undo step. Currently, a heuristic is used to approximate the memory usage, but better solutions could be found if necessary.
❤️ 7 🚀 12
Jacques Lucke added 33 commits 2023-04-13 15:08:31 +02:00
0caab0ee83 Squashed commit of the following: 
commit 33be282c3b415656a4577586dd70999702589a65
Author: Jacques Lucke <jacques@blender.org>
Date:   Wed Mar 22 12:42:53 2023 +0100

    fix after merg

commit 4b9d973bc6c564104e35a641bb64ec7b5c08c5f8
Merge: 2318d0a508 69ba34fa21
Author: Jacques Lucke <jacques@blender.org>
Date:   Wed Mar 22 12:40:23 2023 +0100

    Merge branch 'main' into temp-copy-on-write

commit 2318d0a508
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 20:34:55 2023 +0100

    fix

    Fixed this in the past at some point, but a merge conflict reverted it..

commit ff134afff8
Merge: 3400e55ba9 99506b3d73
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 20:17:06 2023 +0100

    Merge branch 'main' into temp-copy-on-write

commit 3400e55ba9
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 20:10:36 2023 +0100

    cleanup name

commit 36a18db75a
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 19:29:24 2023 +0100

    rename

commit 5f4ca0ca2c
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 19:24:23 2023 +0100

    cleanup

commit 73aa98c62f
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 19:11:28 2023 +0100

    cleanup

commit 27061b704b
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 18:52:30 2023 +0100

    cleanup

commit 39d770fae4
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 18:21:57 2023 +0100

    cleanup

commit 0f9e2af3b0
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 17:49:22 2023 +0100

    fix double free in edit-mesh-undo

commit 657b0dc433
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 17:47:18 2023 +0100

    make destructor virtual

commit f890130a6c
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 17:04:15 2023 +0100

    remove rna integration

commit 195e9c335b
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 17:00:38 2023 +0100

    fix merge error

commit 28faa89077
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 16:30:14 2023 +0100

    cleanup

commit 7ce2820c05
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 15:54:16 2023 +0100

    cleanup

commit 14905927b3
Merge: 80f34eaa4d 92b607d686
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Mar 14 15:53:35 2023 +0100

    Merge branch 'main' into temp-copy-on-write

commit 80f34eaa4d
Merge: 06e1b3f9f6 01480229b1
Author: Jacques Lucke <jacques@blender.org>
Date:   Fri Feb 10 17:01:07 2023 +0100

    Merge branch 'main' into temp-copy-on-write

commit 06e1b3f9f6
Merge: 82ffc03449 43f308f216
Author: Jacques Lucke <jacques@blender.org>
Date:   Wed Feb 8 14:48:47 2023 +0100

    Merge branch 'main' into temp-copy-on-write-customdata

commit 82ffc03449
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Feb 7 19:50:58 2023 +0100

    fix

commit ce46579108
Merge: e71021db06 f5552d759c
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Feb 7 19:50:54 2023 +0100

    Merge branch 'main' into temp-copy-on-write-customdata

commit e71021db06
Merge: ff6f1a0a70 b642dc7bc7
Author: Jacques Lucke <jacques@blender.org>
Date:   Mon Feb 6 00:27:06 2023 +0100

    Merge branch 'master' into temp-copy-on-write-customdata

commit ff6f1a0a70
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Feb 5 23:36:06 2023 +0100

    simplify embedding cow

commit 9c51e093a3
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Feb 5 23:17:32 2023 +0100

    partial fix

commit 1284bdc24c
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Feb 5 22:31:05 2023 +0100

    removing support for saving undo step as actual file

commit a59b848992
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Feb 5 22:22:23 2023 +0100

    fix

commit 4d67393e7e
Merge: 2e9fcdc600 3d6ceb737d
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Feb 5 22:19:52 2023 +0100

    Merge branch 'master' into temp-copy-on-write-customdata

commit 2e9fcdc600
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 22:42:05 2023 +0100

    disable array store for editmesh undo

commit b4ac85084c
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 22:41:40 2023 +0100

    improve rna api

commit 6f4772d170
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 21:53:57 2023 +0100

    cleanup file structure

commit b703fc76c8
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 21:47:48 2023 +0100

    cleanup

commit 60612b84dc
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 21:46:45 2023 +0100

    cleanup

commit a16a60f600
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 21:45:35 2023 +0100

    cleanup

commit 6065a378cf
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 21:43:04 2023 +0100

    use virtual class

commit 53178b94e0
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 31 19:55:17 2023 +0100

    initial rna integration

commit ee675db8f6
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Jan 29 02:16:59 2023 +0100

    store destructor in cow type

commit 8f6b51ac15
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Jan 29 00:20:36 2023 +0100

    cleanup

commit c9477aac6a
Merge: c678fe09e4 e497da5fda
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Jan 29 00:15:55 2023 +0100

    Merge branch 'master' into temp-copy-on-write-customdata

commit c678fe09e4
Author: Jacques Lucke <jacques@blender.org>
Date:   Mon Jan 23 01:48:20 2023 +0100

    cleanup

commit 097c085a4e
Author: Jacques Lucke <jacques@blender.org>
Date:   Mon Jan 23 01:47:04 2023 +0100

    Support copy-on-write in undo system

commit ef1f42865e
Merge: 2b71cf4b31 93a840360a
Author: Jacques Lucke <jacques@blender.org>
Date:   Mon Jan 23 00:55:37 2023 +0100

    Merge branch 'master' into temp-copy-on-write-customdata

commit 2b71cf4b31
Merge: d4ec435d4f 3a2899cc31
Author: Jacques Lucke <jacques@blender.org>
Date:   Sun Jan 22 20:44:29 2023 +0100

    Merge branch 'master' into temp-copy-on-write-customdata

commit d4ec435d4f
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 18:14:36 2023 +0100

    compile fixes

commit ba33242d9a
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 16:35:33 2023 +0100

    progress

commit 66b4840d90
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 16:32:06 2023 +0100

    fix freeing

commit 8cff18da91
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 15:24:49 2023 +0100

    progress

commit b45b522d22
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 15:02:06 2023 +0100

    progress

commit b592d76a8e
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 14:32:56 2023 +0100

    remove CD_FLAG_NOFREE

commit be0fe0433d
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 14:21:54 2023 +0100

    cleanup

commit 3530c63611
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 14:11:36 2023 +0100

    progress

commit c1d1665a79
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 14:01:47 2023 +0100

    progress

commit 3f22933534
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 13:47:35 2023 +0100

    progress

commit c4c9a43072
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 13:42:06 2023 +0100

    progress

commit e3779d12d5
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 13:28:31 2023 +0100

    progress

commit 3ed5d00f7e
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 13:01:10 2023 +0100

    fix

commit 5329d5803e
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 12:50:29 2023 +0100

    progress

commit 51388d401c
Merge: 5a3c8a1b12 ced0021498
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 11:41:08 2023 +0100

    Merge branch 'temp-copy-on-write' into temp-copy-on-write-customdata

commit ced0021498
Merge: 0b7155b5c5 6aa29549e8
Author: Jacques Lucke <jacques@blender.org>
Date:   Sat Jan 21 11:40:42 2023 +0100

    Merge branch 'master' into temp-copy-on-write

commit 5a3c8a1b12
Author: Jacques Lucke <jacques@blender.org>
Date:   Fri Jan 20 23:45:03 2023 +0100

    change custom data api for cow

commit 0b7155b5c5
Author: Jacques Lucke <jacques@blender.org>
Date:   Fri Jan 20 21:59:07 2023 +0100

    fix

commit d76e6862f0
Author: Jacques Lucke <jacques@blender.org>
Date:   Fri Jan 20 21:57:15 2023 +0100

    fix

commit 6b1028d492
Author: Jacques Lucke <jacques@blender.org>
Date:   Fri Jan 20 21:52:16 2023 +0100

    fix

commit dd8af7b46a
Merge: ac439c7eed 244c87dd68
Author: Jacques Lucke <jacques@blender.org>
Date:   Fri Jan 20 21:45:15 2023 +0100

    Merge branch 'master' into temp-copy-on-write

commit ac439c7eed
Merge: 3f3adef4a0 c99d1d5d0d
Author: Jacques Lucke <jacques@blender.org>
Date:   Thu Feb 17 17:23:52 2022 +0100

    Use new copy-on-write system for sharing geometry components.

    Differential Revision: https://developer.blender.org/D14139

commit 3f3adef4a0
Merge: c5b55a84f5 b2867d4365
Author: Jacques Lucke <jacques@blender.org>
Date:   Tue Jan 4 13:38:02 2022 +0100

    Merge branch 'master' into temp-copy-on-write

commit c5b55a84f5
Author: Jacques Lucke <jacques@blender.org>
Date:   Mon Jan 3 16:57:15 2022 +0100

    cleanup

commit 1713a780c5
Author: Jacques Lucke <jacques@blender.org>
Date:   Mon Jan 3 16:55:45 2022 +0100

    progress

commit e0ac932156
Author: Jacques Lucke <jacques@blender.org>
Date:   Mon Jan 3 15:29:23 2022 +0100

    initial data structure
buildbot/vexp-code-patch-coordinator Build done. Details
994bab2934 improve naming
buildbot/vexp-code-patch-coordinator Build done. Details
3f17b7bebb cleanup naming
buildbot/vexp-code-patch-coordinator Build done. Details
7dedd25112 Merge branch 'main' into implicit-sharing-custom-data
buildbot/vexp-code-patch-coordinator Build done. Details
061e3d827a Merge branch 'main' into implicit-sharing-custom-data
buildbot/vexp-code-patch-coordinator Build done. Details
78aa83e235 add clarifying comment in BLI_implicit_sharing.h
Jacques Lucke added 3 commits 2023-04-20 23:00:21 +02:00
Jacques Lucke added 6 commits 2023-04-21 00:00:49 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
03b5d6c4c9 add comments
Jacques Lucke changed title from WIP: Undo: support implicit-sharing in memfile undo step to Undo: support implicit-sharing in memfile undo step 2023-04-21 00:36:29 +02:00
Jacques Lucke commented 2023-04-21 00:36:40 +02:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke requested review from Hans Goudey 2023-04-21 00:36:55 +02:00
Bastien Montagne requested review from Bastien Montagne 2023-04-21 09:59:16 +02:00
Jacques Lucke added 1 commit 2023-04-21 10:32:58 +02:00
Jacques Lucke requested review from Brecht Van Lommel 2023-04-21 10:36:25 +02:00
Jacques Lucke requested review from Sergey Sharybin 2023-04-21 10:36:25 +02:00
Sergey Sharybin requested changes 2023-04-21 10:57:14 +02:00
Sergey Sharybin left a comment
Owner
Copy Link

From reading the description and code the proposed auto-save behavior will cause a lot of frustration for sculpting workflow. Flushing edits to an object could be very slow for hires mesh and multires sculpting. Those extra delays during sculpting were already reported as undesired, and this change seems to make it even worse.

So while it is indeed bad to not include sculpted edits in auto-save I do not find flush to be the proper solution to the problem. Autosave is only used when Blender looses the data (which ideally should not happen), but the periodic slow and expensive flush happens all the time.

From reading the description and code the proposed auto-save behavior will cause a lot of frustration for sculpting workflow. Flushing edits to an object could be very slow for hires mesh and multires sculpting. Those extra delays during sculpting were already reported as undesired, and this change seems to make it even worse. So while it is indeed bad to not include sculpted edits in auto-save I do not find flush to be the proper solution to the problem. Autosave is only used when Blender looses the data (which ideally should not happen), but the periodic slow and expensive flush happens all the time.
Jacques Lucke commented 2023-04-21 11:03:04 +02:00
Author
Member
Copy Link

The main solution I can see here is to just disable auto-save when in certain modes that require flushing, and only autosaving when entering/exiting these modes (depending on when the last auto-save happened).
Auto-saving in a mode where all your changes are not saved anyway does not make sense to me.

The main solution I can see here is to just disable auto-save when in certain modes that require flushing, and only autosaving when entering/exiting these modes (depending on when the last auto-save happened). Auto-saving in a mode where all your changes are not saved anyway does not make sense to me.
Sergey Sharybin commented 2023-04-21 11:12:17 +02:00
Owner
Copy Link

I think this is better short-term alternative.

There are ways to make auto-save work properly for multires sculpt, but they I think are outside of the scope of this patch.

I think this is better short-term alternative. There are ways to make auto-save work properly for multires sculpt, but they I think are outside of the scope of this patch.
Jacques Lucke added 1 commit 2023-04-21 11:19:16 +02:00
Jacques Lucke requested review from Sergey Sharybin 2023-04-21 11:22:45 +02:00
Jacques Lucke added 1 commit 2023-04-21 11:36:56 +02:00
Sergey Sharybin approved these changes 2023-04-21 11:48:51 +02:00
Sergey Sharybin left a comment
Owner
Copy Link

Code and design sides seems good to me now.
Did not have time yet to do user-level testing.

Code and design sides seems good to me now. Did not have time yet to do user-level testing.
Hans Goudey added 4 commits 2023-04-21 20:54:34 +02:00
f0533e434c Avoid the need for a macro and void ** 
BLO_read_shared doesn't actually read the data itself,
so it doesn't need to change its pointer. That's up to the lambda it calls.
Hans Goudey added 1 commit 2023-04-21 21:00:44 +02:00
Hans Goudey added 2 commits 2023-04-21 21:22:32 +02:00
Hans Goudey added 1 commit 2023-04-21 21:23:39 +02:00
Hans Goudey approved these changes 2023-04-21 21:25:28 +02:00
Hans Goudey left a comment
Member
Copy Link

This is working well in my testing. I did find a memory leak once (curve sculpt edit mode, object mode, sculpt mode, strokes, undo, redo) but I haven't been able to reproduce it, so not sure. I've reproduced some of the memory usage improvements too.

This is working well in my testing. I did find a memory leak once (curve sculpt edit mode, object mode, sculpt mode, strokes, undo, redo) but I haven't been able to reproduce it, so not sure. I've reproduced some of the memory usage improvements too.
Brecht Van Lommel approved these changes 2023-04-24 17:39:56 +02:00
Dismissed
Jacques Lucke added 1 commit 2023-04-25 09:25:53 +02:00
Jacques Lucke requested review from Campbell Barton 2023-04-25 09:31:38 +02:00
Campbell Barton commented 2023-05-04 09:32:00 +02:00
Owner
Copy Link

Works well, undo tests pass (from ../lib/tests/ui_simulate/run.py) with ASAN enabled.
Removing USE_WRITE_CRASH_BLEND seems fine too.

Attached updated patch rebased on main.

Works well, undo tests pass (from `../lib/tests/ui_simulate/run.py`) with ASAN enabled. Removing `USE_WRITE_CRASH_BLEND` seems fine too. _Attached updated patch rebased on `main`._
pr_106903_update.diff
28 KiB
Campbell Barton approved these changes 2023-05-04 09:32:24 +02:00
Jacques Lucke added 1 commit 2023-05-04 10:02:28 +02:00
Jacques Lucke added 1 commit 2023-05-09 14:04:56 +02:00
Jacques Lucke added 1 commit 2023-05-25 14:37:23 +02:00
Bastien Montagne requested changes 2023-06-05 17:11:26 +02:00
Dismissed
Bastien Montagne left a comment
Owner
Copy Link

While in general this looks like a nice improvements, there are several issues that makes it not suitable for merge as-is imho.

1. Memory Size Limit

While broken until today (fixed in 855d8d3fa4), Blender has a feature to limit the amount of memory used by undo steps. This gets fully broken by this PR (with the file from #88010, current main with memory limit of 1MB only allows one undo step, and uses about 3.4GB or virtual RAM; this PR code allows up to 10 steps and occupies 4.4GB of virtual RAM).

I think that these shared memory buffers size should be added to the undo step size value. Or maybe only when they become only owned by the memory step(s) ?

2. Autosave Speed

I do not think that current memfile-to-blendfile process should be ditched without further thinking. While posing a Pets Project production file, current main autosave takes between 100 and 220 ms (mostly around 100 ms); this PR makes it between 220 and 300 ms (mostly around 220 ms), that's more than twice slower.

I wonder if it would be possible to store the callback given to BLO_write_shared into the ImplicitSharingInfo (or some derived specialized class maybe)? then you could just serialize these when writing the memfile, instead of having to redo a full complete .blendfile writing.

While in general this looks like a nice improvements, there are several issues that makes it not suitable for merge as-is imho. ## 1. Memory Size Limit While broken until today (fixed in 855d8d3fa4), Blender has a feature to limit the amount of memory used by undo steps. This gets fully broken by this PR (with the file from #88010, current main with memory limit of 1MB only allows one undo step, and uses about 3.4GB or virtual RAM; this PR code allows up to 10 steps and occupies 4.4GB of virtual RAM). I think that these shared memory buffers size should be added to the undo step size value. Or maybe only when they become only owned by the memory step(s) ? ## 2. Autosave Speed I do not think that current memfile-to-blendfile process should be ditched without further thinking. While posing a Pets Project production file, current main autosave takes between 100 and 220 ms (mostly around 100 ms); this PR makes it between 220 and 300 ms (mostly around 220 ms), that's more than twice slower. I wonder if it would be possible to store the callback given to `BLO_write_shared` into the `ImplicitSharingInfo` (or some derived specialized class maybe)? then you could just serialize these when writing the memfile, instead of having to redo a full complete .blendfile writing.
source/blender/blenkernel/intern/customdata.cc Outdated
@ -5190,3 +5157,1 @@
__func__,
structname,
layer.type);
BLO_write_shared(writer, layer.data, layer.sharing_info, [&]() {
Bastien Montagne commented 2023-06-02 15:47:32 +02:00
Owner
Copy Link

I would rather not have such long callbacks as lambda, makes it very difficult to decipher the code imho.

Same for the reader below.

I would rather not have such long callbacks as lambda, makes it very difficult to decipher the code imho. Same for the reader below.
JacquesLucke marked this conversation as resolved
source/blender/blenloader/BLO_read_write.h Outdated
@ -258,0 +299,4 @@
extern "C" {
# endif
#endif
Bastien Montagne commented 2023-06-02 15:50:55 +02:00
Owner
Copy Link

This block of #ifdef __cplusplus is extremely confusing, it closes an extern C block without any comment, then open another one at the end, even with an extra, useless macro check...

I would suggest keeping all C++-only code in its own area, to avoid juggling with that kind of contexts switches, would make the code cleaner, clearer and safer for future changes.

This block of `#ifdef __cplusplus` is extremely confusing, it closes an `extern C` block without any comment, then open another one at the end, even with an extra, useless macro check... I would suggest keeping all C++-only code in its own area, to avoid juggling with that kind of contexts switches, would make the code cleaner, clearer and safer for future changes.
HooglyBoogly marked this conversation as resolved
Bastien Montagne commented 2023-06-05 17:16:05 +02:00
Owner
Copy Link

A benefit of not saving MemFile to disk is reduced risk of corrupted .blend files. This is because in blend-write code we sometimes do things differently when writing an undo step. Code reading .blend files including versioning code currently has to handle loading "normal" files and those created from an undo step (auto-save and quit.blend). This led to bugs a couple of times in the past. The most recent case of this issue was fixed in 43b37fbc93.

I find it weird to claim that this commit avoids issues when these issues should not have existed in the first place.

The fact that a module decided that it was fine to write something completely different depending on whether it was writing undo steps or actual blend file on disk is a violation of current design in itself, and should never have gone in main without proper discussion and checking for alternative solutions.

> A benefit of not saving `MemFile` to disk is reduced risk of corrupted `.blend` files. This is because in blend-write code we sometimes do things differently when writing an undo step. Code reading `.blend` files including versioning code currently has to handle loading "normal" files and those created from an undo step (auto-save and `quit.blend`). This led to bugs a couple of times in the past. The most recent case of this issue was fixed in 43b37fbc93. I find it weird to claim that this commit avoids issues when these issues should not have existed in the first place. The fact that a module decided that it was fine to write something completely different depending on whether it was writing undo steps or actual blend file on disk is a violation of current design in itself, and should never have gone in main without proper discussion and checking for alternative solutions.
Ludvik Koutny commented 2023-08-27 11:10:15 +02:00
Contributor
Copy Link

Hi, is this still planned to make it into 4.0? It's really frustrating issue and 4.0 release is coming ever closer. This has all the worrying signals of a typical Blender rotting patch. There's a message from @mont29 that something should be done (proper discussion and checking for alternative solution), but then there's no one actually taking responsibility and making a plan to follow through. For 2 months already, this is stuck on 4 out of 5 reviewers having approved, and request for changes of one reviewer left without any response, let alone plan of action.

This patch has 57 commits, so obviously lots of effort and time went into it. It'd be shame if it rotted away like many other great patches.

Hi, is this still planned to make it into 4.0? It's really frustrating issue and 4.0 release is coming ever closer. This has all the worrying signals of a typical Blender rotting patch. There's a message from @mont29 that something should be done (proper discussion and checking for alternative solution), but then there's no one actually taking responsibility and making a plan to follow through. For 2 months already, this is stuck on 4 out of 5 reviewers having approved, and request for changes of one reviewer left without any response, let alone plan of action. This patch has 57 commits, so obviously lots of effort and time went into it. It'd be shame if it rotted away like many other great patches.
Daniel Gryningstjerna commented 2023-08-27 16:18:41 +02:00
First-time contributor
Copy Link

Hi, I strongly agree with @Rawalanche and I am also similarly worried that this patch will be forgotten. Getting this patch in is critical and should not be delayed any further if possible. This is a great step in the right direction to finally making Blender more usable in production. Currently Blender's high memory usage and slow undo speed make Blender constantly choke for minutes while doing even very basic operations in bigger scenes.

The current performance and memory usage are in many aspects way behind most other DCC apps, and as much as I love Blender, I find it to be its biggest weakness. Landing this patch would be a big improvement and should be a high priority for the Blender Foundation.

Any news @mont29 if we can expect to see a final review and approval of this patch?

Hi, I strongly agree with @Rawalanche and I am also similarly worried that this patch will be forgotten. Getting this patch in is critical and should not be delayed any further if possible. This is a great step in the right direction to finally making Blender more usable in production. Currently Blender's high memory usage and slow undo speed make Blender constantly choke for minutes while doing even very basic operations in bigger scenes. The current performance and memory usage are in many aspects way behind most other DCC apps, and as much as I love Blender, I find it to be its biggest weakness. Landing this patch would be a big improvement and should be a high priority for the Blender Foundation. Any news @mont29 if we can expect to see a final review and approval of this patch?
Bastien Montagne commented 2023-08-28 10:35:15 +02:00
Owner
Copy Link

Not sure what you guys are expecting from me here? This patch has indeed been dormant, so it's in any case way to late to make it into 4.0. Such major changes in critical areas of Blender need to go in early BCon1 stage. And none of my questions or suggestions have been addressed so far, so...

Not sure what you guys are expecting from me here? This patch has indeed been dormant, so it's in any case _way_ to late to make it into 4.0. Such major changes in critical areas of Blender need to go in early BCon1 stage. And none of my questions or suggestions have been addressed so far, so...
👍 1 😕 2
Daniel Gryningstjerna commented 2023-08-28 20:16:46 +02:00
First-time contributor
Copy Link

Not sure what you guys are expecting from me here? This patch has indeed been dormant, so it's in any case way to late to make it into 4.0. Such major changes in critical areas of Blender need to go in early BCon1 stage. And none of my questions or suggestions have been addressed so far, so...

Alright, that makes sense and thanks for the update, will its unfortunate, I understand that development resources are limited and that you have pointed out valid design critics of this patch that will be needed to be fixed before commited. I am sorry if my comment came across as harsh.

> Not sure what you guys are expecting from me here? This patch has indeed been dormant, so it's in any case _way_ to late to make it into 4.0. Such major changes in critical areas of Blender need to go in early BCon1 stage. And none of my questions or suggestions have been addressed so far, so... Alright, that makes sense and thanks for the update, will its unfortunate, I understand that development resources are limited and that you have pointed out valid design critics of this patch that will be needed to be fixed before commited. I am sorry if my comment came across as harsh.
Ludvik Koutny commented 2023-11-14 12:36:22 +01:00
Contributor
Copy Link

Hi, this was said to be expected to be part of 4.0. Will it be at least part of 4.1?

Hi, this was said to be expected to be part of 4.0. Will it be at least part of 4.1?
Jacques Lucke commented 2023-11-14 12:38:17 +01:00
Author
Member
Copy Link

I want to work on this patch this week again, but can't make any promises yet.

I want to work on this patch this week again, but can't make any promises yet.
👍 2 ❤️ 3 🎉 3
Jacques Lucke force-pushed implicit-sharing-undo from 7886bd84da to 84dd0b2829 2023-11-15 12:54:05 +01:00 Compare
Jakub Jaszewski commented 2023-11-22 15:46:00 +01:00
First-time contributor
Copy Link

It would be great if this patch landed in 4.1.

In my opinion it is far more important than vast majority of new features that landed in the last 6 months.
What is new feature good for if you cannot use it to the full extent because of an UI lag?

I mentioned in the last comment in th bugreport for #98574 this problem also affects instances made with Geometry Nodes. So now the lag can also happen in scenes with way less than 1 mil poly.

It would be great if this patch landed in 4.1. In my opinion it is far more important than vast majority of new features that landed in the last 6 months. What is new feature good for if you cannot use it to the full extent because of an UI lag? I mentioned in the last comment in th bugreport for https://projects.blender.org/blender/blender/issues/98574 this problem also affects instances made with Geometry Nodes. So now the lag can also happen in scenes with way less than 1 mil poly.
👍 1
Hans Goudey added 5 commits 2024-01-15 20:48:28 +01:00
7b8b74ec92 Cleanup: Remove unnecessary custom data layer writing 
These layers are removed in current versions
Hans Goudey added 2 commits 2024-01-16 20:11:39 +01:00
Jacques Lucke added 1 commit 2024-02-23 17:25:38 +01:00
Jacques Lucke added 2 commits 2024-02-29 13:37:32 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
e9c40b8ce8 fixes after merge
Jacques Lucke commented 2024-02-29 14:03:24 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke added 1 commit 2024-02-29 14:11:08 +01:00
Jacques Lucke added 1 commit 2024-02-29 14:17:57 +01:00
Jacques Lucke added 2 commits 2024-02-29 14:24:05 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
3abf7d1d0e Merge branch 'main' into implicit-sharing-undo
Jacques Lucke commented 2024-02-29 14:24:20 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke requested review from Bastien Montagne 2024-02-29 14:45:22 +01:00
Jacques Lucke requested review from Brecht Van Lommel 2024-02-29 14:45:26 +01:00
Bastien Montagne requested changes 2024-02-29 16:09:22 +01:00
Dismissed
source/blender/blenkernel/intern/curves_geometry.cc Outdated
@ -1573,0 +1575,4 @@
BLO_write_shared(
&writer,
this->curve_offsets,
sizeof(int) * this->curve_num,
Bastien Montagne commented 2024-02-29 15:54:55 +01:00
Owner
Copy Link

shouldn't this be * (this->curve_num + 1) ?

shouldn't this be `* (this->curve_num + 1)` ?
JacquesLucke marked this conversation as resolved
source/blender/blenloader/BLO_read_write.hh Outdated
@ -185,0 +194,4 @@
*/
void BLO_write_shared(BlendWriter *writer,
const void *data,
size_t size_in_bytes,
Bastien Montagne commented 2024-02-29 15:59:34 +01:00
Owner
Copy Link

Don't find it super nice to have to manually compute this value here... At the very least, it should be clearly documented what it is actually used for.

In the future, wouldn't it be better and more consistent to also add this info to ImplicitSharingInfo? Although I guess that would not be that simple, so probably not for this PR.

Don't find it super nice to have to manually compute this value here... At the very least, it should be clearly documented what it is actually used for. In the future, wouldn't it be better and more consistent to also add this info to `ImplicitSharingInfo`? Although I guess that would not be that simple, so probably not for this PR.
Jacques Lucke commented 2024-02-29 16:34:49 +01:00
Author
Member
Copy Link

I improved the comments.

I'm not sure if this really belongs into ImplicitSharingInfo. The concept of memory usage seems orthogonal to implicit sharing. We should find a way to avoid to not compute the size again and again for more complex data, but that's not really a problem yet.

I improved the comments. I'm not sure if this really belongs into `ImplicitSharingInfo`. The concept of memory usage seems orthogonal to implicit sharing. We should find a way to avoid to not compute the size again and again for more complex data, but that's not really a problem yet.
👍 1
JacquesLucke marked this conversation as resolved
source/blender/blenloader/BLO_read_write.hh Outdated
@ -245,6 +261,26 @@ void BLO_read_pointer_array(BlendDataReader *reader, void **ptr_p);
/* Misc. */
void BLO_read_shared_impl(BlendDataReader *reader,
Bastien Montagne commented 2024-02-29 15:56:39 +01:00
Owner
Copy Link

Why do we still need these two functions for reading?

Why do we still need these two functions for reading?
Jacques Lucke commented 2024-02-29 16:35:59 +01:00
Author
Member
Copy Link

This _impl function is supposed to be private and shouldn't be used elsewhere, I used the blo_ prefix instead of BLO_ now.

This `_impl` function is supposed to be private and shouldn't be used elsewhere, I used the `blo_` prefix instead of `BLO_` now.
JacquesLucke marked this conversation as resolved
source/blender/blenloader/intern/writefile.cc Outdated
@ -1827,0 +1846,4 @@
return;
}
/* This size is an estimate, but good enough to count data with many users less. */
memfile.size += size_in_bytes / sharing_info->strong_users();
Bastien Montagne commented 2024-02-29 15:46:18 +01:00
Owner
Copy Link

Pretty sure that these lines should be before the return; statement above?

Pretty sure that these lines should be before the `return;` statement above?
JacquesLucke marked this conversation as resolved
source/blender/blenloader/intern/writefile.cc Outdated
@ -1827,0 +1848,4 @@
/* This size is an estimate, but good enough to count data with many users less. */
memfile.size += size_in_bytes / sharing_info->strong_users();
}
memfile.size += size_in_bytes;
Bastien Montagne commented 2024-02-29 15:45:48 +01:00
Owner
Copy Link

Why adding to memfile.size here? Isn't this the 'regular filewrite' case, and size being handled by the internal filewrite logic?

Why adding to `memfile.size` here? Isn't this the 'regular filewrite' case, and size being handled by the internal filewrite logic?
JacquesLucke marked this conversation as resolved
Jacques Lucke added 4 commits 2024-02-29 16:36:39 +01:00
Bastien Montagne approved these changes 2024-02-29 16:43:34 +01:00
Brecht Van Lommel approved these changes 2024-02-29 16:44:22 +01:00
Bastien Montagne commented 2024-02-29 16:46:16 +01:00
Owner
Copy Link

Oh, forgot one other suggestion, would like to have the changes to the autosave system (and the 'crash on save' removal') committed separately from the undo system change itself. While they are a consequence of the implicit sharing usage in undo, they can be done beforehand (with appropriate comment about why). This will reduce the scope and size of the implicit sharing undo commit itself.

Oh, forgot one other suggestion, would like to have the changes to the autosave system (and the 'crash on save' removal') committed separately from the undo system change itself. While they are a consequence of the implicit sharing usage in undo, they can be done beforehand (with appropriate comment about why). This will reduce the scope and size of the implicit sharing undo commit itself.
Jacques Lucke commented 2024-02-29 16:48:07 +01:00
Author
Member
Copy Link

Sure :)

Sure :)
🎉 5
Jacques Lucke added 1 commit 2024-02-29 17:07:53 +01:00
Jacques Lucke merged commit 0e8e219d71 into main 2024-02-29 17:15:09 +01:00
Jacques Lucke deleted branch implicit-sharing-undo 2024-02-29 17:15:11 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Sergey Sharybin
Hans Goudey
Campbell Barton
Bastien Montagne
Brecht Van Lommel
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward 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
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
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
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
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 Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
9 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#106903
No description provided.
Delete Branch "JacquesLucke/blender:implicit-sharing-undo"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?