-
Notifications
You must be signed in to change notification settings - Fork 60
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.
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
Make sure that the ROOT writers enforce consistency for the Frame contents they write #513
Conversation
b3ecd30
to
8fc5d81
Compare
9321d90
to
c82ca5f
Compare
c82ca5f
to
9dbd135
Compare
@andresailer @Zehvogel any thoughts about the contents of the exception messages? In principle the only exception that we can trigger from the k4FWCore PodioOutput is the one where a collection is not available. The reason is that there we are guaranteed to have a consistent set of |
Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>
src/rootUtils.h
Outdated
// Since we are guaranteed to have unique names here, we can just look for | ||
// collisions brute force, which seems to be quickest approach for vector | ||
// sizes we typically have here (few hundred) | ||
for (const auto& id : candidateColls) { | ||
if (std::find(existingColls.begin(), existingColls.end(), id) == existingColls.end()) { | ||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Since we are guaranteed to have unique names here, we can just look for | |
// collisions brute force, which seems to be quickest approach for vector | |
// sizes we typically have here (few hundred) | |
for (const auto& id : candidateColls) { | |
if (std::find(existingColls.begin(), existingColls.end(), id) == existingColls.end()) { | |
return false; | |
} | |
} | |
// Since we are guaranteed to have unique names here, we can just look for | |
// collisions brute force, which seems to be quickest approach for vector | |
// sizes we typically have here (few hundred) | |
for (const auto& id : candidateColls) { | |
if (!std::binary_search(existingColls.begin(), existingColls.end(), id, [](const auto& lhs, const auto& rhs) { std::lexicographical_compare( | |
lhs.begin(), lhs.end(), rhs.begin(), rhs.end(), | |
[](const auto& cl, const auto& cr) { return std::tolower(cl) < std::tolower(cr); }))) { | |
return false; | |
} | |
} |
We can use that existingColls
is ordered, and maybe make a helper function for the comparison lambda, looks like it should be faster.
Edit: Actually they are not sorted for the rntuple so this wouldn't work then without sorting them first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the binary_search
version to benchmarks1 I did before to see if a more involved approach works better for us. It looks like a binary_search
approach without lower-casing things is the best (depending on the number of collections). On the other hand, in most cases this check will probably exit simply because the two vectors have unequal size.
Results with one vector orderd as we do in the ROOTFrameWriter (and the second one unordered). The numbers are the sizes of the vectors. CompareLinear
is the current implementation, CompareBinary
and CompareBinaryLowerCase
use binary_search
. The former one uses binary_search(existingColls.begin(), existingColls.end(), id)
, the second one the proposal from above.
---------------------------------------------------------------------
Benchmark Time CPU Iterations
---------------------------------------------------------------------
CompareLinear/2 11.8 ns 11.8 ns 57782514
CompareLinear/8 50.6 ns 50.5 ns 10000000
CompareLinear/64 1153 ns 1151 ns 603770
CompareLinear/512 61044 ns 60996 ns 11362
CompareBinary/2 27.8 ns 27.8 ns 24459705
CompareBinary/8 142 ns 141 ns 4901529
CompareBinary/64 525 ns 525 ns 1375137
CompareBinary/512 706 ns 705 ns 983974
CompareBinaryLowerCase/2 251 ns 251 ns 2779374
CompareBinaryLowerCase/8 1646 ns 1644 ns 422281
CompareBinaryLowerCase/64 19288 ns 19274 ns 36780
CompareBinaryLowerCase/512 173434 ns 173318 ns 4026
Footnotes
-
Basically what the benchmark does is to select
N
collection names randomly from all the collection names that we collected during Make CollectionIDs a 32bit hash value of the collection name #412. This is done twice, in order to have vectors to compare. In this way the cheap check for the sizes is effectively always skipped and depending onN
the actual loop is executed at least a few times. The biggestN=512
in this case represents the worst case for the runtime as it will do the full N^2 of work. This is not entirely our use case, but it should represent sort of the worst case scenario. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some further checking it turns out that binary_search
without taking into account that existingColls
are lexicographically sorted case insensitve doesn't work because the pre-conditions of binary_search
are broken. Hence, it looks like std::find
and linear searching is still the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for documentation of our short internal discussion. Making the binary_search
approach exit early in case the strings are of unequal sizes, speeds that up quite a bit and it becomes comparable in speed at 64 elements already and scales much better:
-----------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------
CompareLinear/2 11.3 ns 11.3 ns 58053921
CompareLinear/8 48.3 ns 48.3 ns 14333123
CompareLinear/64 1130 ns 1130 ns 618685
CompareLinear/512 60220 ns 60216 ns 11686
CompareBinaryLowerCaseShortCut/2 150 ns 150 ns 4636032
CompareBinaryLowerCaseShortCut/8 498 ns 498 ns 1373998
CompareBinaryLowerCaseShortCut/64 1159 ns 1159 ns 599214
CompareBinaryLowerCaseShortCut/512 6149 ns 6149 ns 111620
Given that our happy path will always have to go through the complete vector to see whether the contents are the same, I have switched to this approach and also made sure that the collection names are sorted accordingly for the RNTuple writer.
BEGINRELEASENOTES
ROOTFrameWriter::writeFrame
andROOTNTupleWriter::writeFrame
that ensure consistent contents for all Frames of a given category. If inconsistent contents are found an exception is thrown. Before these changes this might lead to a crash or to unreadable files. Fixes ROOTFrameWriter can produce unreadable files without warning #382ROOTNTupleWriter
internals to have only one map that keeps track of categories instead of two maps and a set that need to be kept consistent.ENDRELEASENOTES