Repository navigation
[ntuple] outline several definitions from RFieldBase - #16510
Conversation
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
In principle fine, but I think at least for the classes, I'd prefer to move the docstring as well to where the actual code is.
Test Results 14 files 14 suites 3d 13h 5m 58s ⏱️ Results for commit bd0ea62. ♻️ This comment has been updated with latest results. |
| std::size_t Append(const void *from) | ||
| { | ||
| if (~fTraits & kTraitMappable) | ||
| return AppendImpl(from); | ||
|
|
||
| fPrincipalColumn->Append(from); | ||
| return fPrincipalColumn->GetElement()->GetPackedSize(); | ||
| } | ||
| std::size_t Append(const void *from); | ||
|
|
||
| /// Populate a single value with data from the field. The memory location pointed to by to needs to be of the | ||
| /// fitting type. The fast path is conditioned by the field qualifying as simple, i.e. maps as-is | ||
| /// to a single column and has no read callback. | ||
| void Read(NTupleSize_t globalIndex, void *to) | ||
| { | ||
| if (fIsSimple) | ||
| return (void)fPrincipalColumn->Read(globalIndex, to); | ||
|
|
||
| if (fTraits & kTraitMappable) | ||
| fPrincipalColumn->Read(globalIndex, to); | ||
| else | ||
| ReadGlobalImpl(globalIndex, to); | ||
| if (R__unlikely(!fReadCallbacks.empty())) | ||
| InvokeReadCallbacks(to); | ||
| } | ||
|
|
||
| void Read(RClusterIndex clusterIndex, void *to) | ||
| { | ||
| if (fIsSimple) | ||
| return (void)fPrincipalColumn->Read(clusterIndex, to); | ||
|
|
||
| if (fTraits & kTraitMappable) | ||
| fPrincipalColumn->Read(clusterIndex, to); | ||
| else | ||
| ReadInClusterImpl(clusterIndex, to); | ||
| if (R__unlikely(!fReadCallbacks.empty())) | ||
| InvokeReadCallbacks(to); | ||
| } | ||
| void Read(NTupleSize_t globalIndex, void *to); | ||
| void Read(RClusterIndex clusterIndex, void *to); |
There was a problem hiding this comment.
Outlining Append and Read does not allow the compiler to inline them anymore. For simple fields, this inlining allows to directly call into the (principal) RColumn. I'm not sure if this is important, but it's also not clear to me why these functions have to be outlined? They don't seem to use any of the outlined classes / structs...
There was a problem hiding this comment.
No reason other than "they take up space". If it's important for them to be inlined for performance reasons then I'll just put them back there
There was a problem hiding this comment.
Good point! Yes, these two methods should remain inline-able.
9fbc0e9 to
bd0ea62
Compare
3 participants
This Pull request:
moves various definitions outside of
RFieldBasefor improved readabilityChecklist: