Make sure that the ROOT writers enforce consistency for the Frame contents they write

[tmadlener]

BEGINRELEASENOTES

• Introduce checks in ROOTFrameWriter::writeFrame and ROOTNTupleWriter::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 #382
• Refactor ROOTNTupleWriter 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

[tmadlener]

@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 collsToWrite as that is dictated by the framework.

[jmcarcell]

Suggested change

	// 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

[tmadlener]

I added the binary_search version to benchmarks¹ 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

1. 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 on N the actual loop is executed at least a few times. The biggest N=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. ↩

[tmadlener]

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.

[tmadlener]

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.