From a5e7657ceeb6cc5b6a67209ab1120e45ffa39e6f Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Tue, 22 Nov 2022 11:29:12 -0600 Subject: [PATCH] BLI: Remove clamping from span slicing Currently slicing a span clamped the final size so that it would be within bounds of the input. However, in the vast majority of cases that is already the case anyway, and we can use asserts to detect when that assumption fails. The clamping had a performance cost. On a test interpolating a boolean attribute from 1 million curves to 4 million points, removing the clamping saved about 10% of the time. That's an extreme case but this probably slightly improves performance in other cases too. Slicing is used a lot in the new curve code. This commit introduces `slice_safe` which still does the clamping, and uses it in the few places that needed it or where I wasn't sure. --- source/blender/blenlib/BLI_span.hh | 20 +++++++++++-- source/blender/blenlib/tests/BLI_span_test.cc | 2 +- source/blender/draw/intern/draw_command.cc | 4 +-- .../geometry/nodes/node_geo_extrude_mesh.cc | 30 +++++++++---------- 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/source/blender/blenlib/BLI_span.hh b/source/blender/blenlib/BLI_span.hh index adfccd8d6fe..a513c58d6ac 100644 --- a/source/blender/blenlib/BLI_span.hh +++ b/source/blender/blenlib/BLI_span.hh @@ -577,6 +577,22 @@ template class MutableSpan { * is negative. */ constexpr MutableSpan slice(const int64_t start, const int64_t size) const + { + BLI_assert(this->index_range().contains(start)); + BLI_assert(this->index_range().contains(IndexRange(start, size).last())); + return MutableSpan(data_ + start, size); + } + + constexpr MutableSpan slice(IndexRange range) const + { + return this->slice(range.start(), range.size()); + } + + /** + * Returns a contiguous part of the array. This invokes undefined behavior when the start or size + * is negative. Clamps the size of the new new span so it fits in the current one. + */ + constexpr MutableSpan slice_safe(const int64_t start, const int64_t size) const { BLI_assert(start >= 0); BLI_assert(size >= 0); @@ -584,9 +600,9 @@ template class MutableSpan { return MutableSpan(data_ + start, new_size); } - constexpr MutableSpan slice(IndexRange range) const + constexpr MutableSpan slice_safe(IndexRange range) const { - return this->slice(range.start(), range.size()); + return this->slice_safe(range.start(), range.size()); } /** diff --git a/source/blender/blenlib/tests/BLI_span_test.cc b/source/blender/blenlib/tests/BLI_span_test.cc index 2dec84c4206..01f458f3c04 100644 --- a/source/blender/blenlib/tests/BLI_span_test.cc +++ b/source/blender/blenlib/tests/BLI_span_test.cc @@ -143,7 +143,7 @@ TEST(span, SliceLargeN) { Vector a = {1, 2, 3, 4, 5}; Span slice1 = Span(a).slice(3, 100); - MutableSpan slice2 = MutableSpan(a).slice(3, 100); + MutableSpan slice2 = MutableSpan(a).slice_safe(3, 100); EXPECT_EQ(slice1.size(), 2); EXPECT_EQ(slice2.size(), 2); EXPECT_EQ(slice1[0], 4); diff --git a/source/blender/draw/intern/draw_command.cc b/source/blender/draw/intern/draw_command.cc index f7bf8d20547..7a91ecdc108 100644 --- a/source/blender/draw/intern/draw_command.cc +++ b/source/blender/draw/intern/draw_command.cc @@ -420,7 +420,7 @@ std::string DrawMulti::serialize(std::string line_prefix) const intptr_t offset = grp.start; if (grp.back_proto_len > 0) { - for (DrawPrototype &proto : prototypes.slice({offset, grp.back_proto_len})) { + for (DrawPrototype &proto : prototypes.slice_safe({offset, grp.back_proto_len})) { BLI_assert(proto.group_id == group_index); ResourceHandle handle(proto.resource_handle); BLI_assert(handle.has_inverted_handedness()); @@ -432,7 +432,7 @@ std::string DrawMulti::serialize(std::string line_prefix) const } if (grp.front_proto_len > 0) { - for (DrawPrototype &proto : prototypes.slice({offset, grp.front_proto_len})) { + for (DrawPrototype &proto : prototypes.slice_safe({offset, grp.front_proto_len})) { BLI_assert(proto.group_id == group_index); ResourceHandle handle(proto.resource_handle); BLI_assert(!handle.has_inverted_handedness()); diff --git a/source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc b/source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc index 0d3b48fcadd..5a1a4e2c796 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc @@ -272,10 +272,10 @@ static void extrude_mesh_vertices(Mesh &mesh, }); MutableSpan vert_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_POINT); - vert_orig_indices.slice(new_vert_range).fill(ORIGINDEX_NONE); + vert_orig_indices.slice_safe(new_vert_range).fill(ORIGINDEX_NONE); MutableSpan new_edge_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_EDGE); - new_edge_orig_indices.slice(new_edge_range).fill(ORIGINDEX_NONE); + new_edge_orig_indices.slice_safe(new_edge_range).fill(ORIGINDEX_NONE); if (attribute_outputs.top_id) { save_selection_as_attribute( @@ -608,14 +608,14 @@ static void extrude_mesh_edges(Mesh &mesh, } MutableSpan vert_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_POINT); - vert_orig_indices.slice(new_vert_range).fill(ORIGINDEX_NONE); + vert_orig_indices.slice_safe(new_vert_range).fill(ORIGINDEX_NONE); MutableSpan edge_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_EDGE); - edge_orig_indices.slice(connect_edge_range).fill(ORIGINDEX_NONE); - edge_orig_indices.slice(duplicate_edge_range).fill(ORIGINDEX_NONE); + edge_orig_indices.slice_safe(connect_edge_range).fill(ORIGINDEX_NONE); + edge_orig_indices.slice_safe(duplicate_edge_range).fill(ORIGINDEX_NONE); MutableSpan poly_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_FACE); - poly_orig_indices.slice(new_poly_range).fill(ORIGINDEX_NONE); + poly_orig_indices.slice_safe(new_poly_range).fill(ORIGINDEX_NONE); if (attribute_outputs.top_id) { save_selection_as_attribute( @@ -992,15 +992,15 @@ static void extrude_mesh_face_regions(Mesh &mesh, } MutableSpan vert_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_POINT); - vert_orig_indices.slice(new_vert_range).fill(ORIGINDEX_NONE); + vert_orig_indices.slice_safe(new_vert_range).fill(ORIGINDEX_NONE); MutableSpan edge_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_EDGE); - edge_orig_indices.slice(connect_edge_range).fill(ORIGINDEX_NONE); - edge_orig_indices.slice(new_inner_edge_range).fill(ORIGINDEX_NONE); - edge_orig_indices.slice(boundary_edge_range).fill(ORIGINDEX_NONE); + edge_orig_indices.slice_safe(connect_edge_range).fill(ORIGINDEX_NONE); + edge_orig_indices.slice_safe(new_inner_edge_range).fill(ORIGINDEX_NONE); + edge_orig_indices.slice_safe(boundary_edge_range).fill(ORIGINDEX_NONE); MutableSpan poly_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_FACE); - poly_orig_indices.slice(side_poly_range).fill(ORIGINDEX_NONE); + poly_orig_indices.slice_safe(side_poly_range).fill(ORIGINDEX_NONE); if (attribute_outputs.top_id) { save_selection_as_attribute( @@ -1256,14 +1256,14 @@ static void extrude_individual_mesh_faces(Mesh &mesh, }); MutableSpan vert_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_POINT); - vert_orig_indices.slice(new_vert_range).fill(ORIGINDEX_NONE); + vert_orig_indices.slice_safe(new_vert_range).fill(ORIGINDEX_NONE); MutableSpan edge_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_EDGE); - edge_orig_indices.slice(connect_edge_range).fill(ORIGINDEX_NONE); - edge_orig_indices.slice(duplicate_edge_range).fill(ORIGINDEX_NONE); + edge_orig_indices.slice_safe(connect_edge_range).fill(ORIGINDEX_NONE); + edge_orig_indices.slice_safe(duplicate_edge_range).fill(ORIGINDEX_NONE); MutableSpan poly_orig_indices = get_orig_index_layer(mesh, ATTR_DOMAIN_FACE); - poly_orig_indices.slice(side_poly_range).fill(ORIGINDEX_NONE); + poly_orig_indices.slice_safe(side_poly_range).fill(ORIGINDEX_NONE); /* Finally update each extruded polygon's loops to point to the new edges and vertices. * This must be done last, because they were used to find original indices for attribute