Undo: support implicit-sharing in memfile undo step #106903
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
9 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#106903
Loading…
Reference in New Issue
Block a user
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?
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
andBLO_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:
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 inmain
currently). The main remaining copy of the data now is done by rendering code.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:41b10424c7
andf0f304e240
.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.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.
WIP: Undo: support implicit-sharing in memfile undo stepto Undo: support implicit-sharing in memfile undo step@blender-bot build
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.
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.
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.
Code and design sides seems good to me now.
Did not have time yet to do user-level testing.
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.
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
.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 theImplicitSharingInfo
(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.@ -5190,3 +5157,1 @@
__func__,
structname,
layer.type);
BLO_write_shared(writer, layer.data, layer.sharing_info, [&]() {
I would rather not have such long callbacks as lambda, makes it very difficult to decipher the code imho.
Same for the reader below.
@ -258,0 +299,4 @@
extern "C" {
# endif
#endif
This block of
#ifdef __cplusplus
is extremely confusing, it closes anextern 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.
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.
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, 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?
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.
Hi, this was said to be expected to be part of 4.0. Will it be at least part of 4.1?
I want to work on this patch this week again, but can't make any promises yet.
7886bd84da
to84dd0b2829
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.
@blender-bot build
@blender-bot build
@ -1573,0 +1575,4 @@
BLO_write_shared(
&writer,
this->curve_offsets,
sizeof(int) * this->curve_num,
shouldn't this be
* (this->curve_num + 1)
?@ -185,0 +194,4 @@
*/
void BLO_write_shared(BlendWriter *writer,
const void *data,
size_t size_in_bytes,
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.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.@ -245,6 +261,26 @@ void BLO_read_pointer_array(BlendDataReader *reader, void **ptr_p);
/* Misc. */
void BLO_read_shared_impl(BlendDataReader *reader,
Why do we still need these two functions for reading?
This
_impl
function is supposed to be private and shouldn't be used elsewhere, I used theblo_
prefix instead ofBLO_
now.@ -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();
Pretty sure that these lines should be before the
return;
statement above?@ -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;
Why adding to
memfile.size
here? Isn't this the 'regular filewrite' case, and size being handled by the internal filewrite logic?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.
Sure :)