Details

Reviewers

Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)

Commits

rB55a332da6470: Attribute Math: Improve performance of mixer in some cases

Summary

The DefaultMixer has a set of disadvantages:
1 Constructors imply full clearing of the buffer.
2 For some types, finalize is required. It also
works foreveryone, even if only the known
range has changed.
3 mix_in does not allow you to set a new value,
requiringthat everything be cleared beforehand.

This proposal adds the following functionality:
1 Constructor with the specified IndexMask for
preliminary zeroing.
2 set_in method to overwrite the value.
3 finalize with the specified IndexMask to
process only the specified values.

This is useful in situations where you want to use the
DefaultMixer without having to overwrite all the values many times.

Diff Detail

Event Timeline

I think before reviewing, I still need to add comments and change the order for colors

Iliya Katueshenock (Moder) retitled this revision from Attribute_math: DefaultMixer: Empowerment to BKE_attribute_math: DefaultMixer empowerment.Jul 12 2022, 9:57 PM

Merge to master, changes, comments

Merge master
Comment fix

Update master

Iliya Katueshenock (Moder) added a reviewer: Hans Goudey (HooglyBoogly).Jul 16 2022, 5:58 PM

Merge master

These changes seem reasonable, I could see how they open up some optimization opportunities.

source/blender/blenkernel/BKE_attribute_math.hh
198	Suggestion: * \param mask: Only fill the default value for these indices. Other indices in the buffer will be invalid.
200	`clear` -> mask` (A more standard variable name)
244	`ov` -> `of`
246	`IndexMask range` -> `IndexMask mask`
source/blender/blenkernel/intern/attribute_math.cc
132	The existing `finalize` functions should call the new `finalize` functions that take a range, so their logic isn't duplicated.

Hans Goudey (HooglyBoogly) requested changes to this revision.Jul 19 2022, 9:13 PM

This revision now requires changes to proceed.Jul 19 2022, 9:13 PM

Changes
Merge master

Iliya Katueshenock (Moder) planned changes to this revision.Jul 19 2022, 10:22 PM

Iliya Katueshenock (Moder) updated this revision to Diff 53832.Jul 19 2022, 10:25 PM

Replace Span<int64_t>(mask) to mask.indices()

Missing IndexRange replaced with IndexMask

This looks good to me now, except for a couple things inline that I don't think I need to check again. Maybe Jacques has an opinion too though.

source/blender/blenkernel/BKE_attribute_math.hh
194	Might as well use `int64_t` here, since this is a relatively general area (not specific to geometry which uses 32 bit integers elsewhere
315	Always give parameters names (clang tidy has a warning about this, and it's good for people reading the code

Named and specified [[maybe_unused]] for mask in bool mixer
Replaced indexes with int64_t
Added const to all masks parametrs

Merge master

Besides what I've mentioned, I don't have further notes. Don't have to look at it again after these changes.

source/blender/blenkernel/BKE_attribute_math.hh
200	Think this should just be `set` instead of `set_in`. Is this even used anywhere?
238	It seems like the mask should be stored within the mixer so that it doesn't have to be passed on again (which has the risk of going out of sync).

This revision is now accepted and ready to land.Jul 25 2022, 4:06 PM

Until this patch is accepted, an example of using its innovations is only in https://developer.blender.org/D13952

source/blender/blenkernel/BKE_attribute_math.hh
200	I did it similar to mix_in, but this can be changed. An example of usage is in the smooth attribute patch.
238	The main idea is that several different algorithms can work in parallel on different parts of the mixer. Because of this, the mask does not need to be known initially.

set_in -> set
Merge master

Iliya Katueshenock (Moder) added a project: BF Blender.Jul 28 2022, 2:50 PM

Merge master

Merge master

Hans Goudey (HooglyBoogly) added inline comments.Aug 2 2022, 11:51 PM

source/blender/blenkernel/BKE_attribute_math.hh
238	But don't the mast passed to the constructor and the mask passed to `finalize` have to be the same? At least the mask passed to `finalize` should be a subset of the one passed to the constructor. Anything else would be undefined behavior, since we don't initialize the other values of the array in the constructor.

Iliya Katueshenock (Moder) added inline comments.Aug 3 2022, 12:54 AM

source/blender/blenkernel/BKE_attribute_math.hh
238	Yes, it is a helped. But more often than not, you already have a mask, since you have processed the values in it. On the other hand, a common range or mask would mean that some of the values are not used at all. The point is that their processing is only separated into different handlers.

Hans Goudey (HooglyBoogly) added inline comments.Aug 3 2022, 12:56 AM

source/blender/blenkernel/BKE_attribute_math.hh
238	Sorry, I don't really get your point. How does that relate to whether we should store a reference to the mask in the mixer or not?

So to speak. You turn a span of your values into an object for mixing. Now you can use it. But only recording and finalization are available. As a consequence, all operations are local to the single value. As a consequence, they can be grouped and parallelized it. The mask or range belongs to threads.

For example, in the smoothing node, I increased the speed of the entire node by 2 times, making the finalize part of the set and mix threads, but not one common operation

threading::parallel_for(group.mask.index_range(), 1024, [&](const IndexRange range) {
  for (const int index : group.mask.slice(range)) {
    const float count = float(map[index].size());
    const float factor = factors[index] * count;
    buffer.write_a.set_in(index, buffer.read_b[index], count - factor);
    for (const int neighbor : map[index]) {
      buffer.write_a.mix_in(index, buffer.read_b[neighbor], factor);
    }
  }
});
buffer.write_a.finalize(group.mask);

threading::parallel_for(group.mask.index_range(), 1024, [&](const IndexRange range) {
  const IndexMask slice = group.mask.slice(range);
  for (const int index : slice) {
    const float count = float(map[index].size());
    const float factor = factors[index] * count;
    buffer.write_a.set_in(index, buffer.read_b[index], count - factor);
    for (const int neighbor : map[index]) {
      buffer.write_a.mix_in(index, buffer.read_b[neighbor], factor);
    }
  }
  buffer.write_a.finalize(slice);
});

Okay, the code makes sense, thanks. Another option would be multi-threading finalize internally, but keeping the work local and calling finalize for each chunk plays better with CPU cache.

For now, I'm changing the curve cache calculation to see how it might affect other use cases.

Added the use of threading and finalize. I can not judge how much the result is improved by finalize, this case is different from smoothing, where finalize was a very large number of times.

interpolate_to_evaluated and interpolate_to_evaluated_rational (NURBS) canges:
resample curve (20 points, 100.000 resolution): 220~ to 190~ ms

adapt_curve_domain_point_to_curve_impl and adapt_curve_domain_point_to_spline_impl canges:
capture(point) -> capture(spline) color & 100 points & 100000 splines: 150~ to 110~ ms

This revision is now accepted and ready to land.Aug 3 2022, 2:39 AM

Nice improvements!

Jacob Merrill (blueprintrandom) awarded a token.Aug 3 2022, 4:00 AM

When committing, I did a bit of cleanup and grammar cleanup on the comments and commit message. I also removed the changes in the CurveEval/NURBSpline code which will be removed soon.

Yes thank you!

Hans Goudey (HooglyBoogly) added inline comments.Aug 3 2022, 5:15 PM

source/blender/blenkernel/intern/attribute_math.cc
20	I also fixed the zeroing for the color mixers. It should use a zero color like before, instead of the default.

Closed by commit rB55a332da6470: Attribute Math: Improve performance of mixer in some cases (authored by Iliay Katueshenock <Moder>, committed by Hans Goudey (HooglyBoogly)). · Explain WhyAug 3 2022, 5:18 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB55a332da6470: Attribute Math: Improve performance of mixer in some cases.

To be honest, I miscalculated a little. The point is that I compared the resulting time for all nodes in output. But only the operation of the node will improve. from 50~ to 10~ for color (last capture node). For resampling, everything is more or less the same

BKE_attribute_math: DefaultMixer empowerment
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 53748

source/blender/blenkernel/BKE_attribute_math.hh

source/blender/blenkernel/intern/attribute_math.cc

BKE_attribute_math: DefaultMixer empowermentClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 53748

source/blender/blenkernel/BKE_attribute_math.hh

source/blender/blenkernel/intern/attribute_math.cc

BKE_attribute_math: DefaultMixer empowerment
ClosedPublic
Actions

Revision Contents
Changeset List