Add early-exit optimization for LeafNode::resetBackground()

[NBickford-NV]

Hi Academy Software Foundation! This merge request adds a small early-exit check to LeafNode::resetBackground(). This results in a significant speedup when an app merges grids on a node-by-node level using Grid::merge(..., openvdb::MergePolicy::MERGE_NODES).

For a grid where voxels are either equal to the background value, or more than the default tolerance (1e-7) away from the (positive or negative) background value, LeafNode::resetBackground() is a no-op if oldBackground == newBackground or oldBackground == -newBackground:

template<typename T, Index Log2Dim>
inline void
LeafNode<T, Log2Dim>::resetBackground(const ValueType& oldBackground,
                                      const ValueType& newBackground)
{
    if (!this->allocate()) return;

    typename NodeMaskType::OffIterator iter;
    // For all inactive values...
    for (iter = this->mValueMask.beginOff(); iter; ++iter) {
        ValueType &inactiveValue = mBuffer[iter.pos()];
        if (math::isApproxEqual(inactiveValue, oldBackground)) {
            // The condition in the text implies that inactiveValue == oldBackground. So this line is a no-op:
            inactiveValue = newBackground;
        } else if (math::isApproxEqual(inactiveValue, math::negative(oldBackground))) {
            // The condition in the text implies that inactiveValue == math::negative(oldBackground). So this line is a no-op:
            inactiveValue = math::negative(newBackground);
        }
    }
}

This means that we can add this check to skip the for loop entirely:

    if (oldBackground == newBackground || math::negative(oldBackground) == newBackground) return;

On my test application (which computes multiple grids in parallel in such a way that they can use openvdb::MergePolicy::MERGE_NODES), this improves performance of merging together several grids with the same background by about 20x, from ~0.1 seconds to ~0.005 seconds on a ~100 MB volume.

There's one thing to consider here, though: If a grid has values that are ApproxEqual but not exactly equal to the background, then resetBackground(x, +-x)'s current implementation "canonicalizes" these values to be exactly equal to the background. This is undocumented -- the documentation for resetBackground() only talks about values that are exactly equal to the background -- but maybe there's an app out there that relies upon this.

Thank you! Please let me know if there are any changes I can make to this.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: NBickford-NV / name: NBickford (5475fe3, b3985c1)

[Idclip]

Hi @NBickford-NV, thanks for this. Your idea looks good but I don't believe the second condition is valid:

math::negative(oldBackground) == newBackground

The loop still needs to be executed when this is true - inactive values with a different sign will have their sign flipped.

There is a question as to whether or not this comparison should be done by the parent function (because why would you be calling resetBackground with equal values? - it's more of a quirk of the merge functionality running on grids with equal backgrounds). Though interestingly, InternalNodes's already seem to do this optimisation:

if (math::isExactlyEqual(oldBackground, newBackground)) return; 

openvdb/openvdb/openvdb/tree/InternalNode.h

Lines 3011 to 3026 in 6b2d0e3

	InternalNode<ChildT, Log2Dim>::resetBackground(const ValueType& oldBackground,
	const ValueType& newBackground)
	{
	if (math::isExactlyEqual(oldBackground, newBackground)) return;
	for (Index i = 0; i < NUM_VALUES; ++i) {
	if (this->isChildMaskOn(i)) {
	mNodes[i].getChild()->resetBackground(oldBackground, newBackground);
	} else if (this->isValueMaskOff(i)) {
	if (math::isApproxEqual(mNodes[i].getValue(), oldBackground)) {
	mNodes[i].setValue(newBackground);
	} else if (math::isApproxEqual(mNodes[i].getValue(), math::negative(oldBackground))) {
	mNodes[i].setValue(math::negative(newBackground));
	}
	}
	}
	}

I am not a huge fan of the fact that these currently do the approx comparisons, but we could solve this by introducing a third optional tolerance parameter which defaults to Tolerance<Type>::value(). I also find it odd that this handles the negative case (I would have expected this to be a separate method e.g. resetSignedBackground).

Regardless I think the change is a good one, but it doesn't make sense for Internal/Leaf nodes to perform different variants of this optimisation - matching the implementation in InternalNode looks like the correct approach to me

[danrbailey]

I agree with @Idclip, adding a similar early-exit clause similar to the one in InternalNode::resetBackground() would make sense to me.

I will add that this method is known to be slow as it is single-threaded and the poor performance is particularly apparent when merging multiple grids as they need to be merged one-by-one. We introduced a much faster merging method recently that can parallelize across different levels of the tree and across multiple grids at once which significantly improves performance (https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb/openvdb/tools/Merge.h). This new method is now used for all CSG operations, but does not yet support the functionality in Grid::merge(). That is the plan to improve performance of these methods.

[NBickford-NV]

Hi @Idclip ! Yep, I think you're right; resetBackground(v, -v) flips the sign of background values instead of being a no-op. I'll change the line to if (math::isExactlyEqual(oldBackground, newBackground)) return; .

Yep, it turns out that even though InternalNode::resetBackground() has this optimization, InternalNode::merge() calls LeafNode::resetBackground():

https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb/openvdb/tree/InternalNode.h#L2395

[NBickford-NV]

Looks like the macos-nanovdb CI's CMake fails when trying to find TBB. I don't think this merge request modifies that part of the code; anything I can do here? Thanks!

[Idclip]

Looks like the macos-nanovdb CI's CMake fails when trying to find TBB. I don't think this merge request modifies that part of the code; anything I can do here? Thanks!

That error is unrelated to this so no worries there, this just needs a second approval and can be merged