[ntuple] automatic evolution in and out std::atomic

Allow for std::atomic<T> --> T (or compatible) automatic conversion and
vice versa.

Author: jblomer

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

tree/ntuple/src/RField.cxx

@@ -1165,10 +1165,12 @@ std::unique_ptr<ROOT::RFieldBase> ROOT::RAtomicField::CloneImpl(std::string_view
 
 void ROOT::RAtomicField::ReconcileOnDiskField(const RNTupleDescriptor &desc)
 {
-   static const std::vector<std::string> 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::RFieldBase::RValue> ROOT::RAtomicField::SplitValue(const RValue &value) const

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<T> 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<RAtomicField *>(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) {

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

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 <typename T>
    using MyVec = std::vector<T>;

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+;

tree/ntuple/test/ntuple_evolution_type.cxx

@@ -234,6 +234,47 @@ TEST(RNTupleEvolution, Enum)
    EXPECT_EQ(kRenamedCustomEnumVal, ve3(0));
 }
 
+TEST(RNTupleEvolution, CheckAtomic)
+{
+   // TODO(jblomer): enable test with CustomAtomicNotLockFree once linking of libatomic is sorted out.
+
+   FileRaii fileGuard("test_ntuple_evolution_check_atomic.root");
+   {
+      auto model = ROOT::RNTupleModel::Create();
+      auto atomicInt = model->MakeField<std::atomic<std::int32_t>>("atomicInt");
+      auto regularInt = model->MakeField<std::int32_t>("regularInt");
+      auto writer = ROOT::RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath());
+
+      *atomicInt = 7;
+      *regularInt = 13;
+      writer->Fill();
+   }
+
+   auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath());
+
+   auto v1 = reader->GetView<std::atomic<std::int64_t>>("atomicInt");
+   auto v2 = reader->GetView<std::atomic<std::int64_t>>("regularInt");
+   auto v3 = reader->GetView<std::int64_t>("atomicInt");
+
+   try {
+      reader->GetView<std::atomic<std::byte>>("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<CustomAtomicNotLockFree>("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));
+}
+
 TEST(RNTupleEvolution, ArrayAsRVec)
 {
    FileRaii fileGuard("test_ntuple_evolution_array_as_rvec.root");