Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let edm::Wrapper<T> use the constructor T{kUninitialized} #46877

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Let edm::Wrapper<T> use the constructor T{kUninitialized} #46877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2368db5
Fix IOPool/Input test for modified DataFormats/Common
fwyzard Dec 8, 2024
0c38ae5
Rename IOPool/Input test for consistency
fwyzard Dec 8, 2024
e274471
Let edm::Wrapper<T> use the constructor T{kUninitialized}
fwyzard Dec 5, 2024
4c7f02a
Add tests for edm::Wrapper<T> using T{kUninitialized}
fwyzard Dec 7, 2024
74c7d61
Let DeviceProduct<T> use the constructor T{kUninitialized}
fwyzard Dec 7, 2024
4c8434c
Remove the default constructor from PortableCollection
fwyzard Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion DataFormats/Common/interface/DeviceProduct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include <cassert>
#include <memory>

#include "DataFormats/Common/interface/Uninitialized.h"

namespace edm {
class DeviceProductBase {
public:
Expand Down Expand Up @@ -45,7 +47,13 @@ namespace edm {
template <typename T>
class DeviceProduct : public DeviceProductBase {
public:
DeviceProduct() = default;
DeviceProduct()
requires(requires { T(); })
= default;

explicit DeviceProduct(edm::Uninitialized)
requires(requires { T(edm::kUninitialized); })
: data_{edm::kUninitialized} {}

template <typename M, typename... Args>
explicit DeviceProduct(std::shared_ptr<M> metadata, Args&&... args)
Expand Down
19 changes: 19 additions & 0 deletions DataFormats/Common/interface/Uninitialized.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#ifndef DataFormats_Common_interface_Uninitialized_h
#define DataFormats_Common_interface_Uninitialized_h

/* Uninitialized
*
* This is an empty struct used as a tag to signal that a constructor will leave an object (partially) uninitialised,
* with the assumption that it will be overwritten before being used.
* One expected use case is to replace the default constructor used when deserialising objects from a ROOT file.
*/

namespace edm {

struct Uninitialized {};

constexpr inline Uninitialized kUninitialized;

} // namespace edm

#endif // DataFormats_Common_interface_Uninitialized_h
17 changes: 13 additions & 4 deletions DataFormats/Common/interface/Wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ Wrapper: A template wrapper around EDProducts to hold the product ID.

----------------------------------------------------------------------*/

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Common/interface/CMS_CLASS_VERSION.h"
#include "DataFormats/Common/interface/WrapperBase.h"
#include "DataFormats/Common/interface/WrapperDetail.h"
#include "DataFormats/Common/interface/CMS_CLASS_VERSION.h"
#include "DataFormats/Provenance/interface/ProductID.h"
#include "FWCore/Utilities/interface/Visibility.h"

Expand All @@ -25,7 +26,7 @@ namespace edm {
public:
typedef T value_type;
typedef T wrapped_type; // used with the dictionary to identify Wrappers
Wrapper() : WrapperBase(), obj(), present(false) {}
Wrapper() : WrapperBase(), obj{construct_()}, present(false) {}
explicit Wrapper(std::unique_ptr<T> ptr);
Wrapper(Wrapper<T> const& rh) = delete; // disallow copy construction
Wrapper<T>& operator=(Wrapper<T> const&) = delete; // disallow assignment
Expand All @@ -49,6 +50,14 @@ namespace edm {
CMS_CLASS_VERSION(4)

private:
constexpr T construct_() {
if constexpr (requires { T(); }) {
return T();
} else {
return T(edm::kUninitialized);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle we could have a case where T's default constructor is expensive, but it could provide a lightweight T(AllocateForOverwrite) constructor. However, reversing the conditional

if constexpr (requires { T(edm::allocateForOverwrite); }) {
  return T(edm::allocateForOverwrite);
} else {
  return T();
}

would lead to constructor like template <typename U> T(U) to match to the requires clause, and lead to compilation error (we have at least one case here

cmssw/DataFormats/Common/interface/RefToBaseVector.h

Lines 41 to 42 in c5dc5f7

template <class REFV>
explicit RefToBaseVector(REFV const&);

although we really should constrain the REFV type in any case).

For our own types we can add the necessary constraints, but for third party classes it would be more difficult (doable with additional header in CMSSW code, but the setup would be somewhat brittle).

(we could leave this point to a time we have an expensive-to-default-construct data product type, but I felt the case would be useful to be mentioned)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, reversing the conditional ... would ... lead to compilation error

Yes, that's what I tried first, and I run into this kind of errors.

I'm happy to try and implement this inverted approach if you think it worthwhile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we leave it to a later time (usefulness in practice is somewhat unclear, and a straightforward approach wouldn't work well with third party classes).


bool isPresent_() const override { return present; }
std::type_info const& dynamicTypeInfo_() const override { return typeid(T); }
std::type_info const& wrappedTypeInfo_() const override { return typeid(Wrapper<T>); }
Expand Down Expand Up @@ -78,7 +87,7 @@ namespace edm {
};

template <typename T>
Wrapper<T>::Wrapper(std::unique_ptr<T> ptr) : WrapperBase(), obj(), present(ptr.get() != nullptr) {
Wrapper<T>::Wrapper(std::unique_ptr<T> ptr) : WrapperBase(), obj{construct_()}, present(ptr.get() != nullptr) {
if (present) {
obj = std::move(*ptr);
}
Expand All @@ -89,7 +98,7 @@ namespace edm {
Wrapper<T>::Wrapper(Emplace, Args&&... args) : WrapperBase(), obj(std::forward<Args>(args)...), present(true) {}

template <typename T>
Wrapper<T>::Wrapper(T* ptr) : WrapperBase(), present(ptr != 0), obj() {
Wrapper<T>::Wrapper(T* ptr) : WrapperBase(), present(ptr != 0), obj{construct_()} {
std::unique_ptr<T> temp(ptr);
if (present) {
obj = std::move(*ptr);
Expand Down
23 changes: 12 additions & 11 deletions DataFormats/Common/test/Wrapper_t.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,32 @@
/*
* CMSSW
*
*/

#include <cassert>
#include <iostream>
#include <memory>
#include <vector>
#include "catch.hpp"

#include <catch.hpp>

#include "DataFormats/Common/interface/Wrapper.h"

class CopyNoMove {
public:
CopyNoMove() {}
CopyNoMove() = default;
CopyNoMove(CopyNoMove const&) { /* std::cout << "copied\n"; */ }
CopyNoMove& operator=(CopyNoMove const&) { /*std::cout << "assigned\n";*/ return *this; }

private:
};

class MoveNoCopy {
public:
MoveNoCopy() {}
MoveNoCopy() = default;
MoveNoCopy(MoveNoCopy const&) = delete;
MoveNoCopy& operator=(MoveNoCopy const&) = delete;
MoveNoCopy(MoveNoCopy&&) { /* std::cout << "moved\n";*/ }
MoveNoCopy& operator=(MoveNoCopy&&) { /* std::cout << "moved\n";*/ return *this; }
};

private:
class NoDefaultCtor {
public:
NoDefaultCtor() = delete;
NoDefaultCtor(edm::Uninitialized) {}
};

TEST_CASE("test Wrapper", "[Wrapper]") {
Expand All @@ -44,4 +42,7 @@ TEST_CASE("test Wrapper", "[Wrapper]") {
edm::Wrapper<std::vector<double>> wrap3(std::move(thing3));
REQUIRE(wrap3->size() == 10);
REQUIRE(thing3.get() == 0);

auto thing4 = std::make_unique<NoDefaultCtor>(edm::kUninitialized);
edm::Wrapper<NoDefaultCtor> wrap4(std::move(thing4));
}
11 changes: 8 additions & 3 deletions DataFormats/Portable/interface/PortableDeviceCollection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@

#include <alpaka/alpaka.hpp>

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableCollectionCommon.h"
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"
#include "HeterogeneousCore/AlpakaInterface/interface/memory.h"
#include "DataFormats/Portable/interface/PortableCollectionCommon.h"

// generic SoA-based product in device memory
template <typename T, typename TDev, typename = std::enable_if_t<alpaka::isDevice<TDev>>>
Expand All @@ -24,7 +25,9 @@ class PortableDeviceCollection {
using Buffer = cms::alpakatools::device_buffer<TDev, std::byte[]>;
using ConstBuffer = cms::alpakatools::const_device_buffer<TDev, std::byte[]>;

PortableDeviceCollection() = default;
PortableDeviceCollection() = delete;

explicit PortableDeviceCollection(edm::Uninitialized) noexcept {}

PortableDeviceCollection(int32_t elements, TDev const& device)
: buffer_{cms::alpakatools::make_device_buffer<std::byte[]>(device, Layout::computeDataSize(elements))},
Expand Down Expand Up @@ -144,7 +147,9 @@ class PortableDeviceMultiCollection {
}

public:
PortableDeviceMultiCollection() = default;
PortableDeviceMultiCollection() = delete;

explicit PortableDeviceMultiCollection(edm::Uninitialized) noexcept {};

PortableDeviceMultiCollection(int32_t elements, TDev const& device)
: buffer_{cms::alpakatools::make_device_buffer<std::byte[]>(device, Layout<>::computeDataSize(elements))},
Expand Down
5 changes: 4 additions & 1 deletion DataFormats/Portable/interface/PortableDeviceObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <alpaka/alpaka.hpp>

#include "DataFormats/Common/interface/Uninitialized.h"
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"
#include "HeterogeneousCore/AlpakaInterface/interface/memory.h"

Expand All @@ -21,7 +22,9 @@ class PortableDeviceObject {
using Buffer = cms::alpakatools::device_buffer<TDev, Product>;
using ConstBuffer = cms::alpakatools::const_device_buffer<TDev, Product>;

PortableDeviceObject() = default;
PortableDeviceObject() = delete;

PortableDeviceObject(edm::Uninitialized) {}

PortableDeviceObject(TDev const& device)
// allocate global device memory
Expand Down
11 changes: 8 additions & 3 deletions DataFormats/Portable/interface/PortableHostCollection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@

#include <alpaka/alpaka.hpp>

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableCollectionCommon.h"
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"
#include "HeterogeneousCore/AlpakaInterface/interface/host.h"
#include "HeterogeneousCore/AlpakaInterface/interface/memory.h"
#include "DataFormats/Portable/interface/PortableCollectionCommon.h"

// generic SoA-based product in host memory
template <typename T>
Expand All @@ -21,7 +22,9 @@ class PortableHostCollection {
using Buffer = cms::alpakatools::host_buffer<std::byte[]>;
using ConstBuffer = cms::alpakatools::const_host_buffer<std::byte[]>;

PortableHostCollection() = default;
PortableHostCollection() = delete;

explicit PortableHostCollection(edm::Uninitialized) noexcept {};

PortableHostCollection(int32_t elements, alpaka_common::DevHost const& host)
// allocate pageable host memory
Expand Down Expand Up @@ -154,7 +157,9 @@ class PortableHostMultiCollection {
}

public:
PortableHostMultiCollection() = default;
PortableHostMultiCollection() = delete;

explicit PortableHostMultiCollection(edm::Uninitialized) noexcept {};

PortableHostMultiCollection(int32_t elements, alpaka_common::DevHost const& host)
// allocate pageable host memory
Expand Down
5 changes: 4 additions & 1 deletion DataFormats/Portable/interface/PortableHostObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <alpaka/alpaka.hpp>

#include "DataFormats/Common/interface/Uninitialized.h"
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"
#include "HeterogeneousCore/AlpakaInterface/interface/host.h"
#include "HeterogeneousCore/AlpakaInterface/interface/memory.h"
Expand All @@ -19,7 +20,9 @@ class PortableHostObject {
using Buffer = cms::alpakatools::host_buffer<Product>;
using ConstBuffer = cms::alpakatools::const_host_buffer<Product>;

PortableHostObject() = default;
PortableHostObject() = delete;

PortableHostObject(edm::Uninitialized) noexcept {}

PortableHostObject(alpaka_common::DevHost const& host)
// allocate pageable host memory
Expand Down
9 changes: 6 additions & 3 deletions DataFormats/SiPixelClusterSoA/interface/SiPixelClustersDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,20 @@
#define DataFormats_SiPixelClusterSoA_interface_SiPixelClustersDevice_h

#include <cstdint>

#include <alpaka/alpaka.hpp>
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"
#include "DataFormats/SiPixelClusterSoA/interface/SiPixelClustersHost.h"

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableDeviceCollection.h"
#include "DataFormats/SiPixelClusterSoA/interface/SiPixelClustersHost.h"
#include "DataFormats/SiPixelClusterSoA/interface/SiPixelClustersSoA.h"
#include "HeterogeneousCore/AlpakaInterface/interface/CopyToHost.h"
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"

template <typename TDev>
class SiPixelClustersDevice : public PortableDeviceCollection<SiPixelClustersSoA, TDev> {
public:
SiPixelClustersDevice() = default;
SiPixelClustersDevice(edm::Uninitialized) : PortableDeviceCollection<SiPixelClustersSoA, TDev>{edm::kUninitialized} {}

template <typename TQueue>
explicit SiPixelClustersDevice(size_t maxModules, TQueue queue)
Expand Down
6 changes: 4 additions & 2 deletions DataFormats/SiPixelClusterSoA/interface/SiPixelClustersHost.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,18 @@
#define DataFormats_SiPixelClusterSoA_interface_SiPixelClustersHost_h

#include <alpaka/alpaka.hpp>
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableHostCollection.h"
#include "DataFormats/SiPixelClusterSoA/interface/SiPixelClustersSoA.h"
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"

// TODO: The class is created via inheritance of the PortableCollection.
// This is generally discouraged, and should be done via composition.
// See: https://github.com/cms-sw/cmssw/pull/40465#discussion_r1067364306
class SiPixelClustersHost : public PortableHostCollection<SiPixelClustersSoA> {
public:
SiPixelClustersHost() = default;
SiPixelClustersHost(edm::Uninitialized) : PortableHostCollection<SiPixelClustersSoA>{edm::kUninitialized} {}

template <typename TQueue>
explicit SiPixelClustersHost(size_t maxModules, TQueue queue)
Expand Down
5 changes: 4 additions & 1 deletion DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <alpaka/alpaka.hpp>

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableDeviceCollection.h"
#include "DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsSoA.h"
#include "DataFormats/SiPixelRawData/interface/SiPixelErrorCompact.h"
Expand All @@ -14,7 +15,9 @@
template <typename TDev>
class SiPixelDigiErrorsDevice : public PortableDeviceCollection<SiPixelDigiErrorsSoA, TDev> {
public:
SiPixelDigiErrorsDevice() = default;
SiPixelDigiErrorsDevice(edm::Uninitialized)
: PortableDeviceCollection<SiPixelDigiErrorsSoA, TDev>{edm::kUninitialized} {}

template <typename TQueue>
explicit SiPixelDigiErrorsDevice(size_t maxFedWords, TQueue queue)
: PortableDeviceCollection<SiPixelDigiErrorsSoA, TDev>(maxFedWords, queue), maxFedWords_(maxFedWords) {}
Expand Down
4 changes: 3 additions & 1 deletion DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsHost.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <alpaka/alpaka.hpp>

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableHostCollection.h"
#include "DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsSoA.h"
#include "DataFormats/SiPixelRawData/interface/SiPixelErrorCompact.h"
Expand All @@ -13,7 +14,8 @@

class SiPixelDigiErrorsHost : public PortableHostCollection<SiPixelDigiErrorsSoA> {
public:
SiPixelDigiErrorsHost() = default;
SiPixelDigiErrorsHost(edm::Uninitialized) : PortableHostCollection<SiPixelDigiErrorsSoA>{edm::kUninitialized} {}

template <typename TQueue>
explicit SiPixelDigiErrorsHost(int maxFedWords, TQueue queue)
: PortableHostCollection<SiPixelDigiErrorsSoA>(maxFedWords, queue), maxFedWords_(maxFedWords) {}
Expand Down
4 changes: 3 additions & 1 deletion DataFormats/SiPixelDigiSoA/interface/SiPixelDigisDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@

#include <alpaka/alpaka.hpp>

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableDeviceCollection.h"
#include "DataFormats/SiPixelDigiSoA/interface/SiPixelDigisSoA.h"
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"

template <typename TDev>
class SiPixelDigisDevice : public PortableDeviceCollection<SiPixelDigisSoA, TDev> {
public:
SiPixelDigisDevice() = default;
SiPixelDigisDevice(edm::Uninitialized) : PortableDeviceCollection<SiPixelDigisSoA, TDev>{edm::kUninitialized} {}

template <typename TQueue>
explicit SiPixelDigisDevice(size_t maxFedWords, TQueue queue)
: PortableDeviceCollection<SiPixelDigisSoA, TDev>(maxFedWords + 1, queue) {}
Expand Down
4 changes: 3 additions & 1 deletion DataFormats/SiPixelDigiSoA/interface/SiPixelDigisHost.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef DataFormats_SiPixelDigiSoA_interface_SiPixelDigisHost_h
#define DataFormats_SiPixelDigiSoA_interface_SiPixelDigisHost_h

#include "DataFormats/Common/interface/Uninitialized.h"
#include "DataFormats/Portable/interface/PortableHostCollection.h"
#include "DataFormats/SiPixelDigiSoA/interface/SiPixelDigisSoA.h"

Expand All @@ -9,7 +10,8 @@
// See: https://github.com/cms-sw/cmssw/pull/40465#discussion_r1067364306
class SiPixelDigisHost : public PortableHostCollection<SiPixelDigisSoA> {
public:
SiPixelDigisHost() = default;
SiPixelDigisHost(edm::Uninitialized) : PortableHostCollection<SiPixelDigisSoA>{edm::kUninitialized} {}

template <typename TQueue>
explicit SiPixelDigisHost(size_t maxFedWords, TQueue queue)
: PortableHostCollection<SiPixelDigisSoA>(maxFedWords + 1, queue) {}
Expand Down
11 changes: 11 additions & 0 deletions DataFormats/TestObjects/interface/ToyProducts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Toy EDProducts for testing purposes only.
#include "DataFormats/Common/interface/DetSetVectorNew.h"
#include "DataFormats/Common/interface/OwnVector.h"
#include "DataFormats/Common/interface/SortedCollection.h"
#include "DataFormats/Common/interface/Uninitialized.h"
#include "FWCore/Utilities/interface/typedefs.h"

#include <stdexcept>
Expand Down Expand Up @@ -48,6 +49,16 @@ namespace edmtest {
cms_int32_t value;
};

struct MaybeUninitializedIntProduct {
explicit MaybeUninitializedIntProduct(edm::Uninitialized) {}
explicit MaybeUninitializedIntProduct(int i) : value(i) {}
~MaybeUninitializedIntProduct() {}

bool operator==(MaybeUninitializedIntProduct const& rhs) const { return value == rhs.value; }

cms_int32_t value;
};

struct UInt64Product {
explicit UInt64Product(unsigned long long i = 0) : value(i) {}
~UInt64Product() {}
Expand Down
Loading