diff --git a/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx b/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx index a24af5fc9342e..b2b118fbf9b3a 100644 --- a/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx @@ -153,6 +153,7 @@ public: /// The dictionary does not need to be available for this method. /// Needs the full descriptor to look up sub fields. bool IsCustomEnum(const RNTupleDescriptor &desc) const; + bool IsStdAtomic() const; }; // clang-format off diff --git a/tree/ntuple/src/RField.cxx b/tree/ntuple/src/RField.cxx index 39ae9c3e49ff2..bc258f859f61b 100644 --- a/tree/ntuple/src/RField.cxx +++ b/tree/ntuple/src/RField.cxx @@ -1165,10 +1165,12 @@ std::unique_ptr ROOT::RAtomicField::CloneImpl(std::string_view void ROOT::RAtomicField::ReconcileOnDiskField(const RNTupleDescriptor &desc) { - static const std::vector prefixes = {"std::atomic<"}; - - EnsureMatchingOnDiskField(desc, kDiffTypeName).ThrowOnError(); - EnsureMatchingTypePrefix(desc, prefixes).ThrowOnError(); + const auto &fieldDesc = desc.GetFieldDescriptor(GetOnDiskId()); + if (fieldDesc.GetTypeName().rfind("std::atomic<", 0) == 0) { + EnsureMatchingOnDiskField(desc, kDiffTypeName).ThrowOnError(); + } else { + fSubfields[0]->SetOnDiskId(GetOnDiskId()); + } } std::vector ROOT::RAtomicField::SplitValue(const RValue &value) const diff --git a/tree/ntuple/src/RFieldBase.cxx b/tree/ntuple/src/RFieldBase.cxx index e15c4e02c247d..8fdbce1797c84 100644 --- a/tree/ntuple/src/RFieldBase.cxx +++ b/tree/ntuple/src/RFieldBase.cxx @@ -952,6 +952,18 @@ void ROOT::RFieldBase::ConnectPageSource(ROOT::Internal::RPageSource &pageSource if (!fDescription.empty()) throw RException(R__FAIL("setting description only valid when connecting to a page sink")); + if (!fIsArtificial) { + R__ASSERT(fOnDiskId != kInvalidDescriptorId); + // Handle moving from on-disk std::atomic to (compatible of) T in memory centrally because otherwise + // we would need to handle it in each and every ReconcileOnDiskField() + // Note that we have to do this before calling BeforeConnectPageSource(), which already may compare the field + // to its on-disk description. + const auto &desc = pageSource.GetSharedDescriptorGuard().GetRef(); + if (!dynamic_cast(this) && desc.GetFieldDescriptor(GetOnDiskId()).IsStdAtomic()) { + SetOnDiskId(desc.GetFieldDescriptor(GetOnDiskId()).GetLinkIds()[0]); + } + } + auto substitute = BeforeConnectPageSource(pageSource); if (substitute) { const RFieldBase *itr = this; @@ -974,8 +986,8 @@ void ROOT::RFieldBase::ConnectPageSource(ROOT::Internal::RPageSource &pageSource } if (!fIsArtificial) { - R__ASSERT(fOnDiskId != kInvalidDescriptorId); - ReconcileOnDiskField(pageSource.GetSharedDescriptorGuard().GetRef()); + const auto &desc = pageSource.GetSharedDescriptorGuard().GetRef(); + ReconcileOnDiskField(desc); } for (auto &f : fSubfields) { diff --git a/tree/ntuple/src/RNTupleDescriptor.cxx b/tree/ntuple/src/RNTupleDescriptor.cxx index a0976d324a23f..7b65e8877d4e4 100644 --- a/tree/ntuple/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/src/RNTupleDescriptor.cxx @@ -187,6 +187,13 @@ bool ROOT::RFieldDescriptor::IsCustomEnum(const RNTupleDescriptor &desc) const desc.GetFieldDescriptor(subFieldId).GetTypeName()) != std::end(gIntTypeNames); } +bool ROOT::RFieldDescriptor::IsStdAtomic() const +{ + if (fStructure != ROOT::ENTupleStructure::kPlain) + return false; + return (fTypeName.rfind("std::atomic<", 0) == 0); +} + //////////////////////////////////////////////////////////////////////////////// bool ROOT::RColumnDescriptor::operator==(const RColumnDescriptor &other) const diff --git a/tree/ntuple/test/CustomStruct.hxx b/tree/ntuple/test/CustomStruct.hxx index b3dea405e2b25..8ef7a5280f639 100644 --- a/tree/ntuple/test/CustomStruct.hxx +++ b/tree/ntuple/test/CustomStruct.hxx @@ -36,6 +36,11 @@ enum class CustomEnumUInt32 : unsigned int {}; enum class CustomEnumInt64 : long int {}; enum class CustomEnumUInt64 : unsigned long int {}; +// Used for std::atomic tests as an example of a class that is not lock-free. +struct CustomAtomicNotLockFree { + int a[100]; +}; + struct CustomStruct { template using MyVec = std::vector; diff --git a/tree/ntuple/test/CustomStructLinkDef.h b/tree/ntuple/test/CustomStructLinkDef.h index 7bdeac07a8443..de05c54758e3c 100644 --- a/tree/ntuple/test/CustomStructLinkDef.h +++ b/tree/ntuple/test/CustomStructLinkDef.h @@ -13,6 +13,8 @@ #pragma link C++ enum CustomEnumInt64; #pragma link C++ enum CustomEnumUInt64; +#pragma link C++ class CustomAtomicNotLockFree+; + #pragma link C++ class CustomStruct+; #pragma link C++ class DerivedA+; #pragma link C++ class DerivedA2+; diff --git a/tree/ntuple/test/ntuple_evolution_type.cxx b/tree/ntuple/test/ntuple_evolution_type.cxx index e55211a2528b9..02d2969998b0c 100644 --- a/tree/ntuple/test/ntuple_evolution_type.cxx +++ b/tree/ntuple/test/ntuple_evolution_type.cxx @@ -234,6 +234,60 @@ TEST(RNTupleEvolution, Enum) EXPECT_EQ(kRenamedCustomEnumVal, ve3(0)); } +TEST(RNTupleEvolution, CheckAtomic) +{ + EXPECT_FALSE(std::atomic{}.is_lock_free()); + + FileRaii fileGuard("test_ntuple_evolution_check_atomic.root"); + { + auto model = ROOT::RNTupleModel::Create(); + auto atomicInt = model->MakeField>("atomicInt"); + auto regularInt = model->MakeField("regularInt"); + auto atomicClass = model->MakeField>("atomicClass"); + auto regularClass = model->MakeField("regularClass"); + auto writer = ROOT::RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); + + *atomicInt = 7; + *regularInt = 13; + std::fill(std::begin(regularClass->a), std::end(regularClass->a), 137); + *atomicClass = *regularClass; + writer->Fill(); + } + + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + + auto v1 = reader->GetView>("atomicInt"); + auto v2 = reader->GetView>("regularInt"); + auto v3 = reader->GetView("atomicInt"); + + auto v4 = reader->GetView("atomicClass"); + auto v5 = reader->GetView>("regularClass"); + + try { + reader->GetView>("atomicInt"); + FAIL() << "automatic evolution into an invalid atomic inner type should fail"; + } catch (const ROOT::RException &err) { + EXPECT_THAT(err.what(), testing::HasSubstr("incompatible with on-disk field")); + } + + try { + reader->GetView("atomicInt"); + FAIL() << "automatic evolution into an invalid non-atomic inner type should fail"; + } catch (const ROOT::RException &err) { + EXPECT_THAT(err.what(), testing::HasSubstr("incompatible type name for field")); + } + + EXPECT_EQ(7, v1(0)); + EXPECT_EQ(13, v2(0)); + EXPECT_EQ(7, v3(0)); + + EXPECT_EQ(137, v4(0).a[0]); + EXPECT_EQ(137, v4(0).a[99]); + CustomAtomicNotLockFree tmp = v5(0); + EXPECT_EQ(137, tmp.a[0]); + EXPECT_EQ(137, tmp.a[99]); +} + TEST(RNTupleEvolution, ArrayAsRVec) { FileRaii fileGuard("test_ntuple_evolution_array_as_rvec.root");