WIP: Core: use generic copy-on-write system to avoid redundant copies #104478

This patch adds copy-on-write support in a few places. The main goal is to avoid copying data unnecessarily, improving memory usage and performance. For more details, see #95845. This patch is only the culmination of a series of patches mostly @HooglyBoogly worked on. We had to make accessing data in CustomData const-correct by introducing the _for_write functions when accessing layers. Furthermore, the embedded pointers to some special layers like Mesh->mvert had to be deprecated.

This patch includes:

CustomData refactor that allows sharing attribute layers. To properly implement this, the API had to be changed a bit as well.
Integration with the undo system, which fixes #98574.
GeometryComponent uses the same copy-on-write system, instead of what it used before.
AnonymousAttributeID also uses the system, but doesn't really have to (because the data is immutable anyway). It was just easier to use the same system for now.

Possible future uses:

Take ownership of attribute arrays in cycles instead of copying them. This might required changes to the Attribute class in cycles.
Avoid copying arrays to create a GPUVertBuf.

Notes:

This removes BLO_memfile_write_file because the entire file is not serialized for an undo step anymore. Serializing the rest on demand is not really trivial (e.g. for vertex groups). I think it was generally a nice thing that an undo step could be written to disk, but it does not feel absolutely necessary if it gets in the way of better run-time performance. The consequence is that quit.blend is now written using normal file-save and that USE_WRITE_CRASH_BLEND has been removed (never knew it existed before, was someone using that?).
The changes I did in editmesh_undo.cc work right now, but are a bit brittle because they make assumptions about where the data comes from. It should be possible to improve the situation there a bit when layers are not partially freed anymore (currently MEM_freeN(layer->data); only frees the pointer array of e.g. vertex groups).

Performance:

Compared to before, performance should generally be the same or better, depending on what the bottleneck is. I didn't measure a performance regression in the production files I tested.
It is relatively straight forward to build an example file that shows an arbitrarily large speedup, so putting numbers to it are somewhat meaningless. For reference I attached one .blend file that shows that copy-on-write is behaving as expected. The Object Info node in geometry nodes retrieves the geometry from another object and modifies only the position. All other attributes are shared. The modifier evaluation on object B takes about 30ms in main and 3ms with this patch applied.

This patch adds copy-on-write support in a few places. The main goal is to avoid copying data unnecessarily, improving memory usage and performance. For more details, see #95845. This patch is only the culmination of a series of patches mostly @HooglyBoogly worked on. We had to make accessing data in `CustomData` const-correct by introducing the `_for_write` functions when accessing layers. Furthermore, the embedded pointers to some special layers like `Mesh->mvert` had to be deprecated. This patch includes: * `CustomData` refactor that allows sharing attribute layers. To properly implement this, the API had to be changed a bit as well. * Integration with the undo system, which fixes #98574. * `GeometryComponent` uses the same copy-on-write system, instead of what it used before. * `AnonymousAttributeID` also uses the system, but doesn't really have to (because the data is immutable anyway). It was just easier to use the same system for now. Possible future uses: * Take ownership of attribute arrays in cycles instead of copying them. This might required changes to the `Attribute` class in cycles. * Avoid copying arrays to create a `GPUVertBuf`. Notes: * This removes `BLO_memfile_write_file` because the entire file is not serialized for an undo step anymore. Serializing the rest on demand is not really trivial (e.g. for vertex groups). I think it was generally a nice thing that an undo step could be written to disk, but it does not feel absolutely necessary if it gets in the way of better run-time performance. The consequence is that `quit.blend` is now written using normal file-save and that `USE_WRITE_CRASH_BLEND` has been removed (never knew it existed before, was someone using that?). * The changes I did in `editmesh_undo.cc` work right now, but are a bit brittle because they make assumptions about where the data comes from. It should be possible to improve the situation there a bit when layers are not partially freed anymore (currently `MEM_freeN(layer->data);` only frees the pointer array of e.g. vertex groups). Performance: * Compared to before, performance should generally be the same or better, depending on what the bottleneck is. I didn't measure a performance regression in the production files I tested. * It is relatively straight forward to build an example file that shows an arbitrarily large speedup, so putting numbers to it are somewhat meaningless. For reference I attached one .blend file that shows that copy-on-write is behaving as expected. The `Object Info` node in geometry nodes retrieves the geometry from another object and modifies only the position. All other attributes are shared. The modifier evaluation on object `B` takes about 30ms in `main` and 3ms with this patch applied.

cow_test.blend

827 KiB

@blender-bot build

Looks very good! I have some comments about naming and comments and a couple other small things.

I did find a crash though. To reproduce, you can just use the grow/shrink brush on the "Random" curves from the add menu.

Looks very good! I have some comments about naming and comments and a couple other small things. I did find a crash though. To reproduce, you can just use the grow/shrink brush on the "Random" curves from the add menu.

The goal sounds great!

There are some topics, without really particular order.

Performance

In the goal you states memory usage and performance improvements. But in the conclusion part where you show the improvements you are only focusing on the timing information.

How about memory usage?
It is quite weird to not have such metric measured in the context of copy-on-write (which is a resource-sharing technique, with a goal to save memory by avoiding creation of unneeded copies).

In the context topic raised in the #95845 one would expect actually measurable memory improvement. From the dependency graph point of view, the memory consumption is about 2x for meshes which do not have any modifiers. This is because the dependency graph "de-couples" the evaluated mesh from the original, so that the render thread can access it and modify, while artist can modify the original mesh in viewport, without causing any conflicts. Sharing the heavy CustomData arrays between the evaluated state and the original until they gets modified should give very good memory save.

Measuring the difference could be a bit more challenging, and might not be 2x when comparing against 2.79, but is still should be measurable improvement.

The biggest saving is expected to have on a scene with a lot of meshes with modifiers disabled. This is how the layout and some animation files are set up for the performance reasons. For the simplicity of memory tracking you can fake such setup by creating a mesh with few tens of million of vertices (do it as a base mesh, do not have modifiers).

Naming

The copy-on-write is a technique, which takes care of efficient copy/sharing of a modifiable resource. I do not think having it stated in types and variables really showing intent in the best possible way.

To me the more correct name for bCopyOnWriteMixin seems could be bShareableMixing, bImplicitlyShareableMixin, bShareableResourceMixin. It more directly showing the intent of it.

I know in a lot of places there is already cow all over the place. But this is already confusing, and even violating the initial design where the proper naming is to use orig_ and eval_.

API

I did not have time to check things in details yet, and was mainly looking into the BKE_customdata.h, as it is something that many developers interface with, and it was already hard to follow.

Maybe some of the points belong to an inlined comment, but some of them affect multiple functions, so seems to make sense to come to a general consensus.

The new suffix is something that confuses me. Not sure how it is expected to behave if CD_ASSIGN is passes as an alloctype, and maybe naming could be better.

If the CD_ASSIGN is purely internal-only use, maybe there are ways to make it more explicit like so. Maybe it can be moved away from header to an implementation file? Maybe renamed to CD_ASSIGN_INTERNAL?

For the new suffix maybe something like construct_alike? To kinda show it explicitly in the name that it is only "topology" of layers is considered, and not the content.

Also not really sure the difference between CustomData_merge_new and CustomData_copy_new. Some comment/information seems to be missing. Is it than the latter considers the dest to be completely un-initialized?

Another point is the const struct bCopyOnWrite *cow. Either the naming is to be changed, or the definition of the structure.

In a more ideal world, you'd imagine that you pass buffer as a single argument, so that you are not risking "stripping" user-counter away by accident. In the terms of C this will be something like

struct bShareableHeader {
  int64_t user_counter;
};

struct CustomDataLayerData {
  bShareableHeader shareable_header;

  void *data_ptr;
};

and somehow pass CustomDataLayerData as a single argument to the _with_data functions.

File IO and Undo

It would indeed eventually be nice to improve memory usage of undo, doing such intrusive changed (as per the PR description) seems a bit odd to do so early on, and will only make the review even longer and more complicated.

What are the consequences of removing BLO_memfile_write_file on the auto-save?

This is something that is not trivial to guess, and there is potential to cause huge usability problems.

On a quick investigation, it seems that before this change the memfile will always be used, even when in the sculpt mode (the use_memfile depends on the user setting, which does not change in the sculpt mode, and there is no warning printed in the console either).

The benefit of this approach is that the auto-save is fast, and does not block the UI, disturbing artists. The downside is that the sculpt changes done in the current sculpt session are not saved in the autosave.

After this change, the ED_editors_flush_edits is forced to happen. This is not cheap operation when you sculpt on a high-res object. Locking UI for a few seconds at regular time intervals is not something artists will appreciate.

Is there a way to split this change into smaller achivable and verifyable steps? For example, is it possible to get the memory optimization of the initial Mesh copy for the depsgraph's CoW without opening the story of the undo? Maybe even smaller pieces are possible (you mentioned making the CustomData const-correct, i.e.)?

Currently the scope seems to be too big, and there are already big topics which seems to be neglected (at least as per the patch description).

The goal sounds great! There are some topics, without really particular order. **Performance** In the goal you states memory usage and performance improvements. But in the conclusion part where you show the improvements you are only focusing on the timing information. How about memory usage? It is quite weird to not have such metric measured in the context of copy-on-write (which is a resource-sharing technique, with a goal to save memory by avoiding creation of unneeded copies). In the context topic raised in the #95845 one would expect actually measurable memory improvement. From the dependency graph point of view, the memory consumption is about 2x for meshes which do not have any modifiers. This is because the dependency graph "de-couples" the evaluated mesh from the original, so that the render thread can access it and modify, while artist can modify the original mesh in viewport, without causing any conflicts. Sharing the heavy CustomData arrays between the evaluated state and the original until they gets modified should give very good memory save. Measuring the difference could be a bit more challenging, and might not be 2x when comparing against 2.79, but is still should be measurable improvement. The biggest saving is expected to have on a scene with a lot of meshes with modifiers disabled. This is how the layout and some animation files are set up for the performance reasons. For the simplicity of memory tracking you can fake such setup by creating a mesh with few tens of million of vertices (do it as a base mesh, do not have modifiers). **Naming** The copy-on-write is a technique, which takes care of efficient copy/sharing of a modifiable resource. I do not think having it stated in types and variables really showing intent in the best possible way. To me the more correct name for `bCopyOnWriteMixin` seems could be `bShareableMixing`, `bImplicitlyShareableMixin`, `bShareableResourceMixin`. It more directly showing the intent of it. I know in a lot of places there is already `cow` all over the place. But this is already confusing, and even violating the initial design where the proper naming is to use `orig_` and `eval_`. **API** I did not have time to check things in details yet, and was mainly looking into the `BKE_customdata.h`, as it is something that many developers interface with, and it was already hard to follow. Maybe some of the points belong to an inlined comment, but some of them affect multiple functions, so seems to make sense to come to a general consensus. The `new` suffix is something that confuses me. Not sure how it is expected to behave if `CD_ASSIGN` is passes as an `alloctype`, and maybe naming could be better. If the `CD_ASSIGN` is purely internal-only use, maybe there are ways to make it more explicit like so. Maybe it can be moved away from header to an implementation file? Maybe renamed to `CD_ASSIGN_INTERNAL`? For the `new` suffix maybe something like `construct_alike`? To kinda show it explicitly in the name that it is only "topology" of layers is considered, and not the content. Also not really sure the difference between `CustomData_merge_new` and `CustomData_copy_new`. Some comment/information seems to be missing. Is it than the latter considers the `dest` to be completely un-initialized? Another point is the `const struct bCopyOnWrite *cow`. Either the naming is to be changed, or the definition of the structure. In a more ideal world, you'd imagine that you pass buffer as a single argument, so that you are not risking "stripping" user-counter away by accident. In the terms of C this will be something like ``` struct bShareableHeader { int64_t user_counter; }; struct CustomDataLayerData { bShareableHeader shareable_header; void *data_ptr; }; ``` and somehow pass `CustomDataLayerData` as a single argument to the `_with_data` functions. **File IO and Undo** It would indeed eventually be nice to improve memory usage of undo, doing such intrusive changed (as per the PR description) seems a bit odd to do so early on, and will only make the review even longer and more complicated. What are the consequences of removing `BLO_memfile_write_file` on the auto-save? This is something that is not trivial to guess, and there is potential to cause huge usability problems. On a quick investigation, it seems that before this change the memfile will always be used, even when in the sculpt mode (the `use_memfile` depends on the user setting, which does not change in the sculpt mode, and there is no warning printed in the console either). The benefit of this approach is that the auto-save is fast, and does not block the UI, disturbing artists. The downside is that the sculpt changes done in the current sculpt session are not saved in the autosave. After this change, the `ED_editors_flush_edits` is forced to happen. This is not cheap operation when you sculpt on a high-res object. Locking UI for a few seconds at regular time intervals is not something artists will appreciate. ---- Is there a way to split this change into smaller achivable and verifyable steps? For example, is it possible to get the memory optimization of the initial Mesh copy for the depsgraph's CoW without opening the story of the undo? Maybe even smaller pieces are possible (you mentioned making the CustomData const-correct, i.e.)? Currently the scope seems to be too big, and there are already big topics which seems to be neglected (at least as per the patch description).

Thanks for the feedback!

You're totally right that this should be split up a bit more. The patch started out as an experiment to see where copy-on-write can be used, so I added everything at once and just didn't think of splitting it up later on.

I suggest the following steps:

Blenlib changes + changes specific to geometry nodes (use the new type in GeometryComponent and AnonymousAttributeID).
CustomData integration.
Undo integration.

Does that sound reasonable to you? If yes, then I'll split things up this way. Will respond to your more specific comments in the respective pull requests then.

To me the more correct name for bCopyOnWriteMixin seems could be bShareableMixing, bImplicitlyShareableMixin, bShareableResourceMixin. It more directly showing the intent of it.

Renaming bCopyOnWrite to ShareableResource is fine with me (same for the Mixin type). Maybe something like SharedResourceInfo or so could work as well, the name makes it a bit more clear that the type is not necessarily the resource itself, but tracks a potentially separately allocated resource (like an attribute array).

Thanks for the feedback! You're totally right that this should be split up a bit more. The patch started out as an experiment to see where copy-on-write can be used, so I added everything at once and just didn't think of splitting it up later on. I suggest the following steps: 1. Blenlib changes + changes specific to geometry nodes (use the new type in `GeometryComponent` and `AnonymousAttributeID`). 2. `CustomData` integration. 3. Undo integration. Does that sound reasonable to you? If yes, then I'll split things up this way. Will respond to your more specific comments in the respective pull requests then. ----- > To me the more correct name for `bCopyOnWriteMixin` seems could be `bShareableMixing`, `bImplicitlyShareableMixin`, `bShareableResourceMixin`. It more directly showing the intent of it. Renaming `bCopyOnWrite` to `ShareableResource` is fine with me (same for the `Mixin` type). Maybe something like `SharedResourceInfo` or so could work as well, the name makes it a bit more clear that the type is not necessarily the resource itself, but tracks a potentially separately allocated resource (like an attribute array).

For the simplicity of memory tracking you can fake such setup by creating a mesh with few tens of million of vertices (do it as a base mesh, do not have modifiers).

As a side note, I just tested this with a heavily subdivided cube. In main the memory usage is 2.5 GB and in the branch 1.13 GB as shown below (also approximately matches what htop shows me).

> For the simplicity of memory tracking you can fake such setup by creating a mesh with few tens of million of vertices (do it as a base mesh, do not have modifiers). As a side note, I just tested this with a heavily subdivided cube. In `main` the memory usage is 2.5 GB and in the branch 1.13 GB as shown below (also approximately matches what `htop` shows me). ![image](/attachments/3365b87c-b686-4b01-ab49-4d3749599ce6)

image.png

132 KiB

Splitting this up would indeed help reviewing, I haven't looked closely since it's a bit difficult to get the big picture and I agree with the points Sergey makes.

The quit.blend saving changes could also be reviewed separately from the other undo changes.

For naming, I think this should be called either "copy on write" or "implicit sharing" since that's the standard terminology. Just shared / shareable seems not distinguished enough for shared_ptr.

Splitting this up would indeed help reviewing, I haven't looked closely since it's a bit difficult to get the big picture and I agree with the points Sergey makes. The quit.blend saving changes could also be reviewed separately from the other undo changes. For naming, I think this should be called either "copy on write" or "implicit sharing" since that's the standard terminology. Just shared / shareable seems not distinguished enough for `shared_ptr`.

@JacquesLucke The plan sounds good.

The memory saving is fantastic!

@JacquesLucke @HooglyBoogly Any objections of sticking to implicit sharing?

To me it seems to be the most clear from both showing intent, disambiguating it from the name of a technique, and solves the possible confusion with shared_ptr.

P.S. The latter one I personally don't really have issues with. To me the bCopyOnWrite (how it is currently called and intended to be used anyway) is the same as shared_ptr when it comes to the life management and assignment. It "just" adds some extra call to "un-share" the resource. But it is important to keep such fundamental data types and APIs as clear as possible and intuitively understood by everyone.

@JacquesLucke The plan sounds good. The memory saving is fantastic! @JacquesLucke @HooglyBoogly Any objections of sticking to `implicit sharing`? To me it seems to be the most clear from both showing intent, disambiguating it from the name of a technique, and solves the possible confusion with `shared_ptr`. P.S. The latter one I personally don't really have issues with. To me the `bCopyOnWrite` (how it is currently called and intended to be used anyway) is the same as `shared_ptr` when it comes to the life management and assignment. It "just" adds some extra call to "un-share" the resource. But it is important to keep such fundamental data types and APIs as clear as possible and intuitively understood by everyone.

I didn't use the term implicit sharing before and didn't come across it much, but can get used to it. Based on Wikipedia the term indeed seems to mean what we want, so I'm fine with that.

I didn't use the term `implicit sharing` before and didn't come across it much, but can get used to it. Based on [Wikipedia](https://en.wikipedia.org/wiki/Copy-on-write) the term indeed seems to mean what we want, so I'm fine with that.

A patch that only contains the CustomData integration is available here: #106228.

A patch that only contains the `CustomData` integration is available here: #106228.

 @ -170,0 +161,4 @@
  * Initializes a CustomData object with the same layers as source. The data is not copied from the
  * source. Instead, the new layers are initialized using the given `alloctype`.
  */
 void CustomData_copy_new(const struct CustomData *source,

 @ -2234,1 +2259,3 @@
     if ((maxnumber != -1) && (number >= maxnumber)) {
     if ((max_current_type_layer_count != -1) &&
         (current_type_layer_count >= max_current_type_layer_count)) {
       /* Don't merge this layer because the maximum amount of layers of this type is reached. */

 @ -2315,0 +2349,4 @@
  * A #bCopyOnWrite that knows how to free the entire referenced custom data layer (including
  * potentially separately allocated chunks like for vertex groups).
  */
 class CustomDataLayerCOW : public bCopyOnWrite {

 @ -638,3 +638,3 @@
   CustomData_free(&pointcloud->pdata, pointcloud->totpoint);
   pointcloud->totpoint = me->totvert;
   CustomData_merge(&me->vdata, &pointcloud->pdata, CD_MASK_PROP_ALL, CD_DUPLICATE, me->totvert);

 @ -0,0 +14,4 @@
 /**
  * #bCopyOnWrite allows implementing copy-on-write behavior, i.e. it allows sharing read-only data
  * between multiple independend systems (e.g. meshes). The data is only copied when it is shared

WIP: Core: use generic copy-on-write system to avoid redundant copies #104478

Reviewers

Pull request closed

 @ -0,0 +22,4 @@
  * data only has a single owner and is mutable. If it is larger than 1, it is shared and must be
  * logically const.
  *
  * On top of containing a reference count, #bCopyOnWrite also knows how to destruct the referenced

 @ -0,0 +32,4 @@
  *   arrays (e.g. for mesh attributes).
  * - It can be embedded into another struct. For that it's best to use #bCopyOnWriteMixin.
  */
 struct bCopyOnWrite : blender::NonCopyable, blender::NonMovable {

 @ -0,0 +10,4 @@
 namespace blender {
 template<typename T> class COWUser {

 @ -1689,0 +1694,4 @@
     return;
   }
   MemFile &memfile = *writer->wd->mem.written_memfile;
   if (memfile.cow_storage == nullptr) {

 @ -0,0 +63,4 @@
   void remove_user_and_delete_if_last() const
   {
     const int old_user_count = users_.fetch_sub(1, std::memory_order_relaxed);

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Core: use generic copy-on-write system to avoid redundant copies #104478

Reviewers

Pull request closed