[ntuple] automatic evolution in and out std::atomic #19937
Conversation
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
9ce4dd5 to
ac97cad
Compare
Test Results 5 files 5 suites 1d 1h 42m 54s ⏱️ For more details on these failures, see this check. Results for commit e44c16a. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
In principle, the changes look neat and small. I'm wondering about the error case: When reconciling an RAtomicField with something on-disk that is not std::atomic, we unconditionally delegate to the item field. Is that maybe confusing for the error message? (I don't have an immediate idea how to do it better)
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
ac97cad to
73ef349
Compare
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
73ef349 to
ef0feec
Compare
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
ef0feec to
1aec090
Compare
tree/ntuple/test/CMakeLists.txt
Outdated
| @@ -37,7 +37,7 @@ ROOT_GENERATE_DICTIONARY(RNTupleDescriptorDict ${CMAKE_CURRENT_SOURCE_DIR}/RNTup | |||
| DEPENDENCIES RIO) | |||
|
|
|||
| ROOT_ADD_GTEST(ntuple_endian ntuple_endian.cxx LIBRARIES ROOTNTuple) | |||
| ROOT_ADD_GTEST(ntuple_evolution_type ntuple_evolution_type.cxx LIBRARIES ROOTNTuple) | |||
| ROOT_ADD_GTEST(ntuple_evolution_type ntuple_evolution_type.cxx LIBRARIES ROOTNTuple atomic) | |||
There was a problem hiding this comment.
Should this use ROOT_ATOMIC_LIBS? Not sure...
There was a problem hiding this comment.
I think it shouldn't. ROOT_ATOMIC_LIBS contains atomic when it is needed even for the normally lock-free cases, AFAICS.
There was a problem hiding this comment.
That is not my reading. It seems to be literally that ROOT_ATOMIC_LIBS is the result of a test that checks whether std::atomic linking require an 'atomic' library or not.
There was a problem hiding this comment.
On my machine (arch), ROOT_ATOMIC_LIBS is empty. If I use is_lock_free(), I need -latomic. For std::atomic<int> etc. I don't.
There was a problem hiding this comment.
But there is obviously an issue on EL platforms. I'm confused. Perhaps the check for ROOT_ATOMIC_LIBS is incomplete/buggy for arch.
There was a problem hiding this comment.
For now, my suggestion is to merge the PR without testing std::atomic<MyClass> and to add that later once we figured out the linking issue.
There was a problem hiding this comment.
That is fine as long as we keep track either by opening the next/new PR 'right away' or opening an issue.
| if (fieldDesc.GetTypeName().rfind("std::atomic<", 0) == 0) { | ||
| EnsureMatchingOnDiskField(fieldDesc, kDiffTypeName).ThrowOnError(); | ||
| } else { | ||
| fSubfields[0]->SetOnDiskId(GetOnDiskId()); |
There was a problem hiding this comment.
I am missing (not seeing) the part of the code that make sure that the non-atomic value is compatible with the in memory type. For example:
auto v10 = reader->GetView<CustomAtomicNotLockFree>("atomicInt");
in the new test should still throw.
There was a problem hiding this comment.
The new test already tests that reader->GetView<std::atomic<std::byte>>("atomicInt") throws.
The RFieldBase::ConnectPageSource() method is recursive. Checking of the inner type is done when it comes to calling the method on the subfield.
There was a problem hiding this comment.
The new test already tests that reader->GetView<std::atomicstd::byte>("atomicInt") throws.
Is that exercising the branch on line 1101 or 1103?
There was a problem hiding this comment.
You're right, the existing line tests something different. I added reader->GetView<CustomAtomicNotLockFree>("atomicInt") (which throws as expected).
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
3 times, most recently
from
7023151 to
e44c16a
Compare
Allow for std::atomic<T> --> T (or compatible) automatic conversion and vice versa.
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
e44c16a to
f75eb4b
Compare
Allow for std::atomic --> T (or compatible) automatic conversion and vice versa.