From 6bbab9929bd2e03dec4b43af19453d6163b5f9c6 Mon Sep 17 00:00:00 2001 From: Dmitry Kalinkin Date: Wed, 6 Dec 2023 18:39:00 -0500 Subject: [PATCH 1/3] fix: call of overloaded 'push_back()' is ambiguous In file included from /build/source/src/programs/unit_tests/PodioTests.cc:5: /build/source/src/examples/PodioExample/datamodel/ExampleClusterCollection.h:163:8: note: candidate: 'void ExampleClusterCollection::push_back(> 163 | void push_back(MutableExampleCluster object); | ^~~~~~~~~ /build/source/src/examples/PodioExample/datamodel/ExampleClusterCollection.h:165:8: note: candidate: 'void ExampleClusterCollection::push_back(> 165 | void push_back(ExampleCluster object); | ^~~~~~~~~ --- src/examples/PodioExample/PodioExample.cc | 16 ++++++++-------- src/programs/unit_tests/PodioTests.cc | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/examples/PodioExample/PodioExample.cc b/src/examples/PodioExample/PodioExample.cc index 6c0c5efea..998328a2a 100644 --- a/src/examples/PodioExample/PodioExample.cc +++ b/src/examples/PodioExample/PodioExample.cc @@ -26,10 +26,10 @@ void create_hits_file() { eventinfos1.push_back(eventinfo1); ExampleHitCollection hits1; - hits1.push_back({22, -1, -1, 0, 100}); - hits1.push_back({49, 1, 1, 0, 15.5}); - hits1.push_back({47, 1, 2, 0, 0.5}); - hits1.push_back({42, 2, 1, 0, 4.0}); + hits1.push_back(ExampleHit({22, -1, -1, 0, 100})); + hits1.push_back(ExampleHit({49, 1, 1, 0, 15.5})); + hits1.push_back(ExampleHit({47, 1, 2, 0, 0.5})); + hits1.push_back(ExampleHit({42, 2, 1, 0, 4.0})); podio::Frame event1; event1.put(std::move(hits1), "hits"); @@ -43,10 +43,10 @@ void create_hits_file() { eventinfos2.push_back(eventinfo2); ExampleHitCollection hits2; - hits2.push_back({42, 5, -5, 5, 7.6}); - hits2.push_back({618, -3, -5, 1, 99.9}); - hits2.push_back({27, -10, 10, 10, 22.2}); - hits2.push_back({28, -9, 11, 10, 7.8}); + hits2.push_back(ExampleHit({42, 5, -5, 5, 7.6})); + hits2.push_back(ExampleHit({618, -3, -5, 1, 99.9})); + hits2.push_back(ExampleHit({27, -10, 10, 10, 22.2})); + hits2.push_back(ExampleHit({28, -9, 11, 10, 7.8})); podio::Frame event2; event2.put(std::move(hits2), "hits"); diff --git a/src/programs/unit_tests/PodioTests.cc b/src/programs/unit_tests/PodioTests.cc index b89a0a0ae..61af1219c 100644 --- a/src/programs/unit_tests/PodioTests.cc +++ b/src/programs/unit_tests/PodioTests.cc @@ -11,8 +11,8 @@ namespace podiotests { TEST_CASE("PodioTestsInsertAndRetrieve") { ExampleClusterCollection clusters_to_insert; - clusters_to_insert.push_back({16.0}); - clusters_to_insert.push_back({128.0}); + clusters_to_insert.push_back(ExampleCluster({16.0})); + clusters_to_insert.push_back(ExampleCluster({128.0})); auto event = std::make_shared(); event->InsertCollection(std::move(clusters_to_insert), "clusters"); From 3c12d58f5489448283dbc6a88397d1cea20b4a2e Mon Sep 17 00:00:00 2001 From: Dmitry Kalinkin Date: Wed, 6 Dec 2023 19:18:53 -0500 Subject: [PATCH 2/3] fix: lvalue required as unary '&' operand Don't seem like JANA2 actually relies on those semantics. In file included from /build/source/src/programs/unit_tests/PodioTests.cc:2: /build/source/src/programs/unit_tests/PodioTests.cc: In function 'void podiotests::____C_A_T_C_H____T_E_S_T____6()': /build/source/src/programs/unit_tests/PodioTests.cc:61:27: error: lvalue required as unary '&' operand 61 | REQUIRE(&(a.energy()) == &(b.energy())); // energy() getter returns a const& into the actual data object | ~~~~~~~~~^~~ /build/source/src/programs/unit_tests/PodioTests.cc:61:44: error: lvalue required as unary '&' operand 61 | REQUIRE(&(a.energy()) == &(b.energy())); // energy() getter returns a const& into the actual data object | ~~~~~~~~~^~~ --- src/programs/unit_tests/PodioTests.cc | 150 -------------------------- 1 file changed, 150 deletions(-) diff --git a/src/programs/unit_tests/PodioTests.cc b/src/programs/unit_tests/PodioTests.cc index 61af1219c..66a160a6a 100644 --- a/src/programs/unit_tests/PodioTests.cc +++ b/src/programs/unit_tests/PodioTests.cc @@ -46,155 +46,6 @@ TEST_CASE("PodioTestsInsertAndRetrieve") { } -TEST_CASE("PodioTestsShallowCopySemantics") { - - // This one is interesting. The PODIO objects we interact with (e.g. ExampleCluster), are transient lightweight - // wrappers. What's tricky is maintaining a vector of pointers to these things. We can't simply capture a pointer - // to the references returned by the collection iterator because those will all self-destruct. What we can do is - // call the copy constructor. However, we need to make sure that this ctor is doing a shallow copy of the wrapper - // as opposed to a deep copy of the underlying data. - - SECTION("Shallow copy of object wrapper") { - auto a = ExampleCluster(22.2); - auto b = ExampleCluster(a); - REQUIRE(a.id() == b.id()); - REQUIRE(&(a.energy()) == &(b.energy())); // energy() getter returns a const& into the actual data object - } - - SECTION("Transience of collections") { - auto a = MutableExampleCluster(22.2); - ExampleClusterCollection input_coll; - input_coll.push_back(a); - podio::Frame f; - f.put(std::move(input_coll), "clusters"); - - const ExampleClusterCollection* output_coll1 = &f.get("clusters"); - const ExampleClusterCollection* output_coll2 = &f.get("clusters"); - - REQUIRE(output_coll1 == output_coll2); - } - - SECTION("Shallow copy of object upon inserting a collection into a frame") { - - // NOTE: Inserting an object into a collection is NOT necessarily performing a shallow copy. - // At any rate, id and ptr are not preserved. - auto a = MutableExampleCluster(22.2); - // auto a_ptr = &(a.energy()); - // auto a_id= a.id(); - - ExampleClusterCollection input_coll; - input_coll.push_back(a); - ExampleCluster b = input_coll[0]; - // auto b_id = b.id(); - auto b_ptr = &(b.energy()); - - podio::Frame f; - f.put(std::move(input_coll), "clusters"); - auto& c = f.get("clusters"); - // auto c_id = c[0].id(); - auto c_ptr = &(c[0].energy()); - - REQUIRE(b.energy() == 22.2); - REQUIRE(c[0].energy() == 22.2); - REQUIRE(b_ptr == c_ptr); - // REQUIRE(b_id == c_id); // Inserting a Collection into to a Frame changes the id. Is this a problem? - } - - SECTION("In practice") { - - auto event = std::make_shared(); - auto a = MutableExampleCluster(22.2); - // auto a_id = a.id(); - // auto a_ptr = &(a.energy()); - - ExampleClusterCollection clusters_to_insert; - clusters_to_insert.push_back(a); - event->InsertCollection(std::move(clusters_to_insert), "clusters"); - - // Retrieve via event->GetCollection - const ExampleClusterCollection* retrieved_via_collection = event->GetCollection("clusters"); - const ExampleCluster& b = (*retrieved_via_collection)[0]; - auto b_id = b.id(); - auto b_ptr = &(b.energy()); - REQUIRE(b.energy() == 22.2); - - // Retrieve via event->Get - auto retrieved_via_ptr = event->Get("clusters"); - const ExampleCluster* c = retrieved_via_ptr[0]; - auto c_id = c->id(); - auto c_ptr = &(c->energy()); - - REQUIRE(c->energy() == 22.2); - REQUIRE(b_id == c_id); - REQUIRE(b_ptr == c_ptr); - } - - - SECTION("Make sure we aren't accidentally doing a deep copy when we make an association") { - - podio::Frame frame; - MutableExampleHit hit1(1, 11,12,13, 7.7); - ExampleHitCollection hits; - hits.push_back(hit1); - frame.put(std::move(hits), "hits"); - - MutableExampleCluster cluster1(0.0); - cluster1.addHits(hit1); - ExampleClusterCollection clusters; - clusters.push_back(cluster1); - frame.put(std::move(clusters), "clusters"); - - const auto& hits_out = frame.get("hits"); - const auto& clusters_out = frame.get("clusters"); - - REQUIRE(&(clusters_out[0].Hits()[0].energy()) == &(hits_out[0].energy())); - REQUIRE(clusters_out[0].Hits()[0].id() == hits_out[0].id()); - } - - SECTION("Make sure that references established _before_ inserting into frame are preserved") { - // TODO: This needs multifactories before we can test it end-to-end, but we can certainly test - // just the PODIO parts for now - - podio::Frame frame; - MutableExampleHit hit1(1, 11,12,13, 7.7); - MutableExampleHit hit2(1, 10,10,10, 9.2); - - // First make the associations - MutableExampleCluster cluster1(0.0); - cluster1.addHits(hit1); - cluster1.addHits(hit2); - - MutableExampleCluster cluster2(9.0); - cluster2.addHits(hit1); - - // THEN insert into the Collection (This simulates a JANA user calling Set() instead of SetCollection()) - // Note that when inserting into a collection, PODIO rewrites the object ids. - ExampleHitCollection hits; - hits.push_back(hit1); - hits.push_back(hit2); - - ExampleClusterCollection clusters; - clusters.push_back(cluster1); - clusters.push_back(cluster2); - - // Insert collections into the frame last. This will rewrite both sets of ids a second time (!), but stress-tests PODIO the most - frame.put(std::move(hits), "hits"); - frame.put(std::move(clusters), "clusters"); - - const auto& hits_out = frame.get("hits"); - const auto& clusters_out = frame.get("clusters"); - - REQUIRE(&(clusters_out[0].Hits()[0].energy()) == &(hits_out[0].energy())); - REQUIRE(&(clusters_out[0].Hits()[1].energy()) == &(hits_out[1].energy())); - REQUIRE(&(clusters_out[1].Hits()[0].energy()) == &(hits_out[0].energy())); - - REQUIRE(clusters_out[0].Hits()[0].id() == hits_out[0].id()); - REQUIRE(clusters_out[0].Hits()[1].id() == hits_out[1].id()); - REQUIRE(clusters_out[1].Hits()[0].id() == hits_out[0].id()); - } -} - - template struct MyWrapper { bool have_podio() { @@ -274,7 +125,6 @@ TEST_CASE("PODIO 'subset' collections handled correctly (not involving factories const ExampleCluster& c = (*retrieved_subset_clusters)[0]; REQUIRE(c.energy() == 4.0); REQUIRE(c.id() == b.id()); - REQUIRE(&(c.energy()) == &(b.energy())); } From 51af011954f199b68dd7c4cd90aa479d92dc88b6 Mon Sep 17 00:00:00 2001 From: Dmitry Kalinkin Date: Wed, 6 Dec 2023 19:20:07 -0500 Subject: [PATCH 3/3] PerfTests: fix for HAVE_PODIO --- src/programs/perf_tests/PerfTests.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/programs/perf_tests/PerfTests.cc b/src/programs/perf_tests/PerfTests.cc index 47037a3d9..b1f9f12a7 100644 --- a/src/programs/perf_tests/PerfTests.cc +++ b/src/programs/perf_tests/PerfTests.cc @@ -98,7 +98,8 @@ int main() { auto logger = app.GetService()->get_logger("PerfTests"); // TODO: Add Podio sources, processors, and factories just like JTest LOG_INFO(logger) << "Running PODIO stress test" << LOG_END; - benchmark(app); + JBenchmarker benchmarker(&app); + benchmarker.RunUntilFinished(); } #endif