Skip to content

Commit

Permalink
perf: Avoid allocations in TrapezoidBounds::inside (acts-project#3705)
Browse files Browse the repository at this point in the history
Looking at some flame graphs I spotted some allocations in `TrapezoidBounds::inside` which should not be necessary. It does not look like it had a big impact.

Apart from that I decided to replace the generic template argument for `insidePolygon` with a span.

---

This pull request includes several changes to the `Acts` library, focusing on improving the handling of vertex data by using `std::span` and enhancing the boundary check logic. The most important changes include the introduction of `std::span` for vertex containers, the addition of necessary includes, and the modification of boundary check methods to use `std::span`.

### Improvements to vertex handling:

* [`Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp`](diffhunk://#diff-afebd1f0fa546e69aed4734d1011f475e9ece065b5529419c7031fc64a2e5aa6L75-R77): Changed the `insidePolygon` function to use `std::span<const Vector2>` for vertex containers.
* [`Core/include/Acts/Surfaces/detail/VerticesHelper.hpp`](diffhunk://#diff-575a21d724214109cec5c36d8f8fc9884ee879564b9f887b481e4cc08c3b294aL177-R179): Modified the `computeClosestPointOnPolygon` function to use `std::span<const Vector2>` for vertex containers.

### Inclusion of necessary headers:

* [`Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp`](diffhunk://#diff-afebd1f0fa546e69aed4734d1011f475e9ece065b5529419c7031fc64a2e5aa6R13-R14): Added the `#include <span>` directive.
* [`Core/include/Acts/Surfaces/detail/VerticesHelper.hpp`](diffhunk://#diff-575a21d724214109cec5c36d8f8fc9884ee879564b9f887b481e4cc08c3b294aR16): Added the `#include <span>` directive.
* [`Core/src/Surfaces/DiamondBounds.cpp`](diffhunk://#diff-602202d907779fe1a0701875345ed35bb225e2431186cb09e845fb622ce259b4R11): Added the `#include "Acts/Definitions/Algebra.hpp"` directive.

### Boundary check logic enhancements:

* [`Core/src/Surfaces/DiamondBounds.cpp`](diffhunk://#diff-602202d907779fe1a0701875345ed35bb225e2431186cb09e845fb622ce259b4L25-R36): Updated the `inside` method to use an array of `Vector2` for vertices and the `insidePolygon` function.
* [`Core/src/Surfaces/TrapezoidBounds.cpp`](diffhunk://#diff-0db048c0e724a9a5dc84f75e864ddb1f21dac518388c5a51de11a0d72cfd8ce2L92-R92): Changed the vertices container from `std::vector` to an array of `Vector2` in the `inside` method.
  • Loading branch information
andiwand authored and Rosie-Hasan committed Nov 13, 2024
1 parent 8173d1e commit a7fb2b0
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 8 deletions.
5 changes: 3 additions & 2 deletions Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

#include "Acts/Surfaces/detail/VerticesHelper.hpp"

#include <span>

namespace Acts::detail {

/// Check if a point is inside a box.
Expand Down Expand Up @@ -72,8 +74,7 @@ inline bool insideAlignedBox(const Vector2& lowerLeft,
/// @param jacobianOpt The Jacobian to transform the distance to Cartesian
///
/// @return True if the point is inside the polygon.
template <typename Vector2Container>
inline bool insidePolygon(const Vector2Container& vertices,
inline bool insidePolygon(std::span<const Vector2> vertices,
const BoundaryTolerance& tolerance,
const Vector2& point,
const std::optional<SquareMatrix2>& jacobianOpt) {
Expand Down
4 changes: 2 additions & 2 deletions Core/include/Acts/Surfaces/detail/VerticesHelper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <algorithm>
#include <cmath>
#include <span>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -174,9 +175,8 @@ bool onHyperPlane(const std::vector<Vector3>& vertices,
ActsScalar tolerance = s_onSurfaceTolerance);

/// Calculate the closest point on the polygon.
template <typename Vector2Container>
inline Vector2 computeClosestPointOnPolygon(const Vector2& point,
const Vector2Container& vertices,
std::span<const Vector2> vertices,
const SquareMatrix2& metric) {
auto squaredNorm = [&](const Vector2& x) {
return (x.transpose() * metric * x).value();
Expand Down
6 changes: 4 additions & 2 deletions Core/src/Surfaces/ConvexPolygonBounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "Acts/Surfaces/ConvexPolygonBounds.hpp"

#include "Acts/Definitions/Algebra.hpp"
#include "Acts/Surfaces/BoundaryTolerance.hpp"
#include "Acts/Surfaces/detail/BoundaryCheckHelper.hpp"

Expand Down Expand Up @@ -51,8 +52,9 @@ Acts::ConvexPolygonBounds<Acts::PolygonDynamic>::type() const {
bool Acts::ConvexPolygonBounds<Acts::PolygonDynamic>::inside(
const Acts::Vector2& lposition,
const Acts::BoundaryTolerance& boundaryTolerance) const {
return detail::insidePolygon(m_vertices, boundaryTolerance, lposition,
std::nullopt);
return detail::insidePolygon(
std::span<const Vector2>(m_vertices.data(), m_vertices.size()),
boundaryTolerance, lposition, std::nullopt);
}

std::vector<Acts::Vector2> Acts::ConvexPolygonBounds<
Expand Down
13 changes: 12 additions & 1 deletion Core/src/Surfaces/DiamondBounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "Acts/Surfaces/DiamondBounds.hpp"

#include "Acts/Definitions/Algebra.hpp"
#include "Acts/Surfaces/BoundaryTolerance.hpp"
#include "Acts/Surfaces/detail/BoundaryCheckHelper.hpp"

Expand All @@ -22,7 +23,17 @@ Acts::SurfaceBounds::BoundsType Acts::DiamondBounds::type() const {
bool Acts::DiamondBounds::inside(
const Acts::Vector2& lposition,
const Acts::BoundaryTolerance& boundaryTolerance) const {
return detail::insidePolygon(vertices(), boundaryTolerance, lposition,
// Vertices starting at lower left (min rel. phi)
// counter-clockwise
double x1 = get(DiamondBounds::eHalfLengthXnegY);
double y1 = get(DiamondBounds::eHalfLengthYneg);
double x2 = get(DiamondBounds::eHalfLengthXzeroY);
double y2 = 0.;
double x3 = get(DiamondBounds::eHalfLengthXposY);
double y3 = get(DiamondBounds::eHalfLengthYpos);
Vector2 vertices[] = {{-x1, -y1}, {x1, -y1}, {x2, y2},
{x3, y3}, {-x3, y3}, {-x2, y2}};
return detail::insidePolygon(vertices, boundaryTolerance, lposition,
std::nullopt);
}

Expand Down
2 changes: 1 addition & 1 deletion Core/src/Surfaces/TrapezoidBounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ bool Acts::TrapezoidBounds::inside(

// at this stage, the point can only be in the triangles
// run slow-ish polygon check
std::vector<Acts::Vector2> vertices = {
Vector2 vertices[] = {
{-hlXnY, -hlY}, {hlXnY, -hlY}, {hlXpY, hlY}, {-hlXpY, hlY}};
return detail::insidePolygon(vertices, boundaryTolerance, extPosition,
std::nullopt);
Expand Down

0 comments on commit a7fb2b0

Please sign in to comment.