Skip to content

Commit

Permalink
refactor: Do not allocate memory if the surface is connected with a d…
Browse files Browse the repository at this point in the history
…etector el… (#3069)

If the surface has an associated detector element its internal transform is never accessed consuming memory for no-reason. If the transform is a unique_ptr which is released in such cases, the footprint should be reduced
  • Loading branch information
junggjo9 authored Jul 3, 2024
1 parent b7a3e59 commit bd98f56
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 13 deletions.
4 changes: 2 additions & 2 deletions Core/include/Acts/Surfaces/Surface.hpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// This file is part of the Acts project.
//
// Copyright (C) 2016-2020 CERN for the benefit of the Acts project
// Copyright (C) 2016-2024 CERN for the benefit of the Acts project
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
Expand Down Expand Up @@ -490,7 +490,7 @@ class Surface : public virtual GeometryObject,

/// Transform3 definition that positions
/// (translation, rotation) the surface in global space
Transform3 m_transform = Transform3::Identity();
std::unique_ptr<const Transform3> m_transform{};

/// Pointer to the a DetectorElementBase
const DetectorElementBase* m_associatedDetElement{nullptr};
Expand Down
2 changes: 1 addition & 1 deletion Core/src/Geometry/CylinderLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Acts::CylinderLayer::CylinderLayer(
auto cVolumeBounds = std::make_shared<CylinderVolumeBounds>(
*CylinderSurface::m_bounds, thickness);
// @todo rotate around x for the avePhi if you have a sector
m_representingVolume = std::make_unique<Volume>(m_transform, cVolumeBounds);
m_representingVolume = std::make_unique<Volume>(*m_transform, cVolumeBounds);

// associate the layer to the surface
CylinderSurface::associateLayer(*this);
Expand Down
3 changes: 2 additions & 1 deletion Core/src/Geometry/DiscLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ Acts::DiscLayer::DiscLayer(const Transform3& transform,
auto rVolumeBounds =
std::make_shared<CylinderVolumeBounds>(*rBounds, thickness);
// @todo rotate around x for the avePhi if you have a sector
m_representingVolume = std::make_unique<Volume>(m_transform, rVolumeBounds);
m_representingVolume =
std::make_unique<Volume>(*m_transform, rVolumeBounds);
}
// associate the layer to the layer surface itself
DiscSurface::associateLayer(*this);
Expand Down
3 changes: 2 additions & 1 deletion Core/src/Surfaces/PlaneSurface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ Acts::PlaneSurface::PlaneSurface(const GeometryContext& gctx,

Acts::PlaneSurface::PlaneSurface(const Vector3& center, const Vector3& normal)
: RegularSurface(), m_bounds(nullptr) {
m_transform = CurvilinearSurface(center, normal).transform();
m_transform = std::make_unique<Transform3>(
CurvilinearSurface(center, normal).transform());
}

Acts::PlaneSurface::PlaneSurface(std::shared_ptr<const PlanarBounds> pbounds,
Expand Down
25 changes: 17 additions & 8 deletions Core/src/Surfaces/Surface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,25 @@ std::array<std::string, Acts::Surface::SurfaceType::Other>
"Cone", "Cylinder", "Disc", "Perigee", "Plane", "Straw", "Curvilinear"};

Acts::Surface::Surface(const Transform3& transform)
: GeometryObject(), m_transform(transform) {}
: GeometryObject(), m_transform(std::make_unique<Transform3>(transform)) {}

Acts::Surface::Surface(const DetectorElementBase& detelement)
: GeometryObject(), m_associatedDetElement(&detelement) {}

Acts::Surface::Surface(const Surface& other)
: GeometryObject(other),
std::enable_shared_from_this<Surface>(),
m_transform(other.m_transform),
m_surfaceMaterial(other.m_surfaceMaterial) {}
m_associatedDetElement(other.m_associatedDetElement),
m_surfaceMaterial(other.m_surfaceMaterial) {
if (other.m_transform) {
m_transform = std::make_unique<Transform3>(*other.m_transform);
}
}

Acts::Surface::Surface(const GeometryContext& gctx, const Surface& other,
const Transform3& shift)
: GeometryObject(),
m_transform(shift * other.transform(gctx)),
m_transform(std::make_unique<Transform3>(shift * other.transform(gctx))),
m_surfaceMaterial(other.m_surfaceMaterial) {}

Acts::Surface::~Surface() = default;
Expand Down Expand Up @@ -156,7 +160,11 @@ Acts::Surface& Acts::Surface::operator=(const Surface& other) {
if (&other != this) {
GeometryObject::operator=(other);
// detector element, identifier & layer association are unique
m_transform = other.m_transform;
if (other.m_transform) {
m_transform = std::make_unique<Transform3>(*other.m_transform);
} else {
m_transform.reset();
}
m_associatedLayer = other.m_associatedLayer;
m_surfaceMaterial = other.m_surfaceMaterial;
m_associatedDetElement = other.m_associatedDetElement;
Expand All @@ -182,7 +190,8 @@ bool Acts::Surface::operator==(const Surface& other) const {
return false;
}
// (e) compare transform values
if (!m_transform.isApprox(other.m_transform, 1e-9)) {
if (m_transform && other.m_transform &&
!m_transform->isApprox((*other.m_transform), 1e-9)) {
return false;
}
// (f) compare material
Expand Down Expand Up @@ -240,7 +249,7 @@ const Acts::Transform3& Acts::Surface::transform(
if (m_associatedDetElement != nullptr) {
return m_associatedDetElement->transform(gctx);
}
return m_transform;
return *m_transform;
}

bool Acts::Surface::insideBounds(const Vector2& lposition,
Expand Down Expand Up @@ -337,7 +346,7 @@ void Acts::Surface::assignDetectorElement(
m_associatedDetElement = &detelement;
// resetting the transform as it will be handled through the detector element
// now
m_transform = Transform3::Identity();
m_transform.reset();
}

void Acts::Surface::assignSurfaceMaterial(
Expand Down

0 comments on commit bd98f56

Please sign in to comment.