Option to request a CustomData layer for write without preserving existing data #117179

Eugene-Kuznetsov · 2024-01-16T17:42:48+01:00

Eugene-Kuznetsov commented

2024-01-16 17:42:48 +01:00

This adds an optional flag to CustomData_get_layer_named_for_write to signal that the caller is not interested in the layer's current content.

(Separated from #116545)

This adds an optional flag to CustomData_get_layer_named_for_write to signal that the caller is not interested in the layer's current content. (Separated from #116545)

Eugene-Kuznetsov referenced this pull request

2024-01-16 17:43:46 +01:00

Improved Surface Deform performance #116545

Iliya Katushenock requested review from Hans Goudey 2024-01-16 17:57:46 +01:00

Hans Goudey reviewed 2024-01-16 19:18:15 +01:00

Hans Goudey left a comment

Thanks for the PR! We have for_write_only in the attribute API for the same reason, it does make a difference. And it makes sense that we should be able to get that benefit without using the attribute API as well. To me the (false) is enough to show that there's something "special" going on.

That said, this would be much more convincing with some performance numbers included. There are also plenty of other places to add this option-- mesh and point cloud accessors for example. I'm happy to add support in other places after this though.

Thanks for the PR! We have `for_write_only` in the attribute API for the same reason, it does make a difference. And it makes sense that we should be able to get that benefit without using the attribute API as well. To me the `(false)` is enough to show that there's something "special" going on. That said, this would be much more convincing with some performance numbers included. There are also plenty of other places to add this option-- mesh and point cloud accessors for example. I'm happy to add support in other places after this though.

source/blender/blenkernel/BKE_curves.hh Outdated

						
				@ -193,3 +193,3 @@

				  Span<float3> positions() const;

				  MutableSpan<float3> positions_for_write();

				  MutableSpan<float3> positions_for_write(bool preserve=true);

Hans Goudey commented

2024-01-16 19:12:02 +01:00

I'd suggest configuring your editor to run clang format whenever you save the file, that would save a lot of time!

Eugene-Kuznetsov marked this conversation as resolved

source/blender/blenkernel/intern/curves_geometry.cc Outdated

						
				@ -248,4 +249,4 @@

				  if (data != nullptr) {

				    return {data, num};

				  }

				  data = (T *)CustomData_add_layer_named(&custom_data, type, CD_SET_DEFAULT, num, name.c_str());

Hans Goudey commented

2024-01-16 19:14:17 +01:00

If preserve is false, you can use CD_CONSTRUCT instead of CD_SET_DEFAULT. That will allow using malloc instead of calloc in most cases.

If `preserve` is false, you can use `CD_CONSTRUCT` instead of `CD_SET_DEFAULT`. That will allow using `malloc` instead of `calloc` in most cases.

Eugene-Kuznetsov marked this conversation as resolved

Hans Goudey commented

2024-01-16 19:30:57 +01:00

It might be worth using the same semantics as the attribute API actually: flipping the value and calling it for_write_only instead.
It's also worth documenting (somewhere?) that all values have to be written to in this case.

It might be worth using the same semantics as the attribute API actually: flipping the value and calling it `for_write_only` instead. It's also worth documenting (somewhere?) that _all_ values have to be written to in this case.

Hans Goudey requested changes 2024-01-19 19:48:39 +01:00

Dismissed

Hans Goudey left a comment

Requesting changes based on my previous comment.

Eugene-Kuznetsov force-pushed customdata_preserve_flag from 46c1c6ecf1 to 331c1d8f52

2024-01-27 00:20:23 +01:00

Compare

Eugene-Kuznetsov commented

2024-01-27 00:45:35 +01:00

Here's what I see from this specific change in #116545, using the test file attached there, and timing either the entire function 4f91c770ce/source/blender/nodes/geometry/nodes/node_geo_deform_curves_on_surface.cc (L328) or specifically the call to positions_for_write:

with 'false':

Timer '<positions_for_write>DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 956 ns, Min: 660 ns, Last: 830 ns 80 calls)
Timer 'DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 19.68 ms, Min: 18.04 ms, Last: 19.44 ms 70 calls)

with 'true':

Timer '<positions_for_write>DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 4.04 ms, Min: 3.95 ms, Last: 4.09 ms 60 calls)
Timer 'DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 23.94 ms, Min: 22.66 ms, Last: 23.76 ms 60 calls)

that's about 15% shaved off the node execution time.

Here's what I see from this specific change in #116545, using the test file attached there, and timing either the entire function https://projects.blender.org/blender/blender/src/commit/4f91c770cefe1384d2397160c320a29fed44dbbd/source/blender/nodes/geometry/nodes/node_geo_deform_curves_on_surface.cc#L328 or specifically the call to positions_for_write: with 'false': Timer '<positions_for_write>DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 956 ns, Min: 660 ns, Last: 830 ns 80 calls) Timer 'DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 19.68 ms, Min: 18.04 ms, Last: 19.44 ms 70 calls) with 'true': Timer '<positions_for_write>DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 4.04 ms, Min: 3.95 ms, Last: 4.09 ms 60 calls) Timer 'DeformCurvesOnSurface(74282, 74282, 1199404)': (Average: 23.94 ms, Min: 22.66 ms, Last: 23.76 ms 60 calls) that's about 15% shaved off the node execution time.

Eugene-Kuznetsov force-pushed customdata_preserve_flag from 331c1d8f52 to 0ccb374d64

2024-03-04 21:01:57 +01:00

Compare

Eugene-Kuznetsov referenced this pull request

2024-03-04 21:25:28 +01:00

Improved Surface Deform performance #116545

Eugene-Kuznetsov referenced this pull request

2024-03-05 20:21:30 +01:00

Improved Surface Deform performance #116545

Hans Goudey commented

2024-03-05 20:28:12 +01:00

Just to be clear, from my perspective this would need some changes to be merged in main.

preserve should change to for_write_only for consistency with the attribute API, defaulted to false
It shouldn't only be bke::CurvesGeometry::positions_for_write() affected, there are lots of similar functions across the geometry types

Just to be clear, from my perspective this would need some changes to be merged in main. - `preserve` should change to `for_write_only` for consistency with the attribute API, defaulted to false - It shouldn't only be `bke::CurvesGeometry::positions_for_write()` affected, there are lots of similar functions across the geometry types

Eugene-Kuznetsov force-pushed customdata_preserve_flag from 0ccb374d64 to f7a2ce8079

2024-03-06 04:09:38 +01:00

Compare

Hans Goudey requested changes 2024-03-06 05:21:36 +01:00

Dismissed

Hans Goudey left a comment

This change grew quite a bit more than I hoped TBH. I'd rather not include create_if_missing in this PR at all-- it's an orthogonal change. Templated CustomData API functions aren't related either, I would also prefer not to combine that with the goal of this PR. And I don't think this should need to include the CPPType for MDeformVert. That's not added yet so far on purpose.

This change grew quite a bit more than I hoped TBH. I'd rather not include `create_if_missing` in this PR at all-- it's an orthogonal change. Templated CustomData API functions aren't related either, I would also prefer not to combine that with the goal of this PR. And I don't think this should need to include the `CPPType` for `MDeformVert`. That's not added yet so far on purpose.

Eugene-Kuznetsov force-pushed customdata_preserve_flag from f7a2ce8079 to 0ccb374d64

2024-03-06 08:02:56 +01:00

Compare

Eugene-Kuznetsov commented

2024-03-06 08:17:20 +01:00

Without the templates, it will not be all that shorter, because, in situations where for_write_only may be set, it is often as part of a pattern

ptr = CustomData_get_layer_named_for_write(...);
    if (ptr == nullptr) {
   CustomData_add_layer_named(...);

so I have to touch at least two lines in every such place anyway. Templates make these patterns shorter and less error-prone.

I've reverted the changes. I'm going to change preserve -> for_write_only next time I have time to work on this.

Without the templates, it will not be all that shorter, because, in situations where for_write_only may be set, it is often as part of a pattern ``` ptr = CustomData_get_layer_named_for_write(...); if (ptr == nullptr) { CustomData_add_layer_named(...); ``` so I have to touch at least two lines in every such place anyway. Templates make these patterns shorter and less error-prone. I've reverted the changes. I'm going to change preserve -> for_write_only next time I have time to work on this.

Eugene-Kuznetsov changed title from ~~Option to request a CustomData layer for write without preserving existing data~~ to WIP: Option to request a CustomData layer for write without preserving existing data

2024-03-06 08:20:52 +01:00

Eugene-Kuznetsov force-pushed customdata_preserve_flag from 0ccb374d64 to 96947ed5ff

2024-03-06 23:17:54 +01:00

Compare

Eugene-Kuznetsov changed title from ~~WIP: Option to request a CustomData layer for write without preserving existing data~~ to Option to request a CustomData layer for write without preserving existing data

2024-03-07 20:43:53 +01:00

Hans Goudey requested changes 2024-03-08 17:13:03 +01:00

Hans Goudey left a comment

I'm actually having trouble finding places where it's correct to use this. There is a problem with your change in #116545. Calling positions_for_write() may remove a user from the existing array if it is non-mutable. If another thread removes the last user after that, while it's still being used by the surface deform node, that results in a use-after free. Generally we have to be careful keeping around a pointer to the old read-only copy of an attribute after calling "for_write".

Besides that case, most places seem to be building geometry from scratch where this isn't helpful. It would be nice to have a few uses of this option in this PR just to show its utility.

I'm actually having trouble finding places where it's correct to use this. There is a problem with your change in #116545. Calling `positions_for_write()` may remove a user from the existing array if it is non-mutable. If another thread removes the last user after that, while it's still being used by the surface deform node, that results in a use-after free. Generally we have to be careful keeping around a pointer to the old read-only copy of an attribute after calling "for_write". Besides that case, most places seem to be building geometry from scratch where this isn't helpful. It would be nice to have a few uses of this option in this PR just to show its utility.

Jacques Lucke referenced this pull request

2024-03-17 11:52:37 +01:00

WIP: Attributes: avoid unnecessary data copies by generalizing attribute unshare policy #119588

Eugene-Kuznetsov commented

2024-03-18 05:51:13 +01:00

These are the locations where for_write seems to be called for (as far as I understand the code):

source/blender/blenkernel/intern/DerivedMesh.cc:406
source/blender/blenkernel/intern/data_transfer.cc:1057 if (mix_factor != 1.0f || mix_weights)
source/blender/blenkernel/intern/mesh_normals.cc:1532
source/blender/modifiers/intern/MOD_cloth.cc:101
source/blender/render/intern/multires_bake.cc:500..508

possibly some places in grease pencil code.

This is helpful even when building geometry from scratch, since it allows to bypass default-initialization. Any time the user expects to overwrite the entire buffer returned by CustomData_layer_named_for_write() or get_mutable_attribute(), there's a performance benefit from letting the API know about it.

These are the locations where for_write seems to be called for (as far as I understand the code): source/blender/blenkernel/intern/DerivedMesh.cc:406 source/blender/blenkernel/intern/data_transfer.cc:1057 if (mix_factor != 1.0f || mix_weights) source/blender/blenkernel/intern/mesh_normals.cc:1532 source/blender/modifiers/intern/MOD_cloth.cc:101 source/blender/render/intern/multires_bake.cc:500..508 possibly some places in grease pencil code. This is helpful even when building geometry from scratch, since it allows to bypass default-initialization. Any time the user expects to overwrite the entire buffer returned by CustomData_layer_named_for_write() or get_mutable_attribute(), there's a performance benefit from letting the API know about it.

Eugene-Kuznetsov force-pushed customdata_preserve_flag from 234b20dbd3 to 1e4f708c4b

2024-04-02 07:18:53 +02:00

Compare

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u customdata_preserve_flag:Eugene-Kuznetsov-customdata_preserve_flag

git checkout Eugene-Kuznetsov-customdata_preserve_flag

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Option to request a CustomData layer for write without preserving existing data #117179

Checkout