From 90dc7c2c504497c64582271efbaece1fcc70eacd Mon Sep 17 00:00:00 2001 From: Nicolas Morales Date: Tue, 15 Aug 2023 10:24:40 -0700 Subject: [PATCH 1/8] #1668: add reproducer test for phase stats issue --- .../unit/collection/test_lb_lite.extended.cc | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/unit/collection/test_lb_lite.extended.cc b/tests/unit/collection/test_lb_lite.extended.cc index 6fdad450cc..a241e5f3df 100644 --- a/tests/unit/collection/test_lb_lite.extended.cc +++ b/tests/unit/collection/test_lb_lite.extended.cc @@ -150,6 +150,41 @@ TEST_F(TestLB, test_lb_1) { } } +TEST_F(TestLB, test_lb_phase_stats) { + auto const& this_node = theContext()->getNode(); + auto const& range = Index1D(32); + auto proxy = theCollection()->constructCollective( + range, "test_lb_phases_stats" + ); + + for (int i = 0; i < num_iter; i++) { + auto cur_time = vt::timing::getCurrentTime(); + + vt::runInEpochCollective([=]{ + proxy.broadcastCollective(i); + }); + + auto total_time = vt::timing::getCurrentTime() - cur_time; + if (this_node == 0) { + fmt::print("iteration: iter={},time={}\n", i, total_time); + } + + vt::thePhase()->nextPhaseCollective(); + } + + auto proxy2 = theCollection()->constructCollective( + range, "test_lb_phases_stats_2" + ); + + for (int i = 0; i < num_iter; i++) { + vt::runInEpochCollective([=]{ + proxy2.broadcastCollective(i); + }); + + vt::thePhase()->nextPhaseCollective(); + } +} + // TEST_F(TestLB, test_lb_multi_1) { // auto const& this_node = theContext()->getNode(); // if (this_node == 0) { From 3e8c5031d967532d25c379026fbdb504b801374a Mon Sep 17 00:00:00 2001 From: Nicolas Morales Date: Tue, 15 Aug 2023 11:51:49 -0700 Subject: [PATCH 2/8] #1668: move phase update to any insertion of elements, not just insertable. Ensure that other tests pass --- src/vt/elm/elm_lb_data.cc | 11 +++++++---- src/vt/vrt/collection/manager.impl.h | 7 +++---- tests/unit/collection/test_lb_data_retention.cc | 3 ++- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/vt/elm/elm_lb_data.cc b/src/vt/elm/elm_lb_data.cc index b35d8ebd5e..ef061c407d 100644 --- a/src/vt/elm/elm_lb_data.cc +++ b/src/vt/elm/elm_lb_data.cc @@ -149,10 +149,13 @@ void ElementLBData::updatePhase(PhaseType const& inc) { // Access all table entries for current phase, to ensure presence even // if they're left empty - phase_timings_[cur_phase_]; - subphase_timings_[cur_phase_]; - phase_comm_[cur_phase_]; - subphase_comm_[cur_phase_]; + if ( cur_phase_ != 0 ) + { + phase_timings_[cur_phase_]; + subphase_timings_[cur_phase_]; + phase_comm_[cur_phase_]; + subphase_comm_[cur_phase_]; + } } void ElementLBData::resetPhase() { diff --git a/src/vt/vrt/collection/manager.impl.h b/src/vt/vrt/collection/manager.impl.h index fa4c3b254b..5cfbd5ea8d 100644 --- a/src/vt/vrt/collection/manager.impl.h +++ b/src/vt/vrt/collection/manager.impl.h @@ -1181,6 +1181,9 @@ bool CollectionManager::insertCollectionElement( elm_holder->insert(idx, typename Holder::InnerHolder{ std::move(vc) }); + auto raw_ptr = elm_holder->lookup(idx).getRawPtr(); + raw_ptr->getLBData().resetPhase(); + raw_ptr->getLBData().updatePhase(thePhase()->getCurrentPhase()); if (is_migrated_in) { theLocMan()->getCollectionLM(proxy)->entityImmigrated( @@ -1681,10 +1684,6 @@ void CollectionManager::insert( if (insert_node == this_node and proceed_with_insertion) { auto cons_fn = detail::InsertMsgDispatcher::makeCons(insert_msg); makeCollectionElement(untyped_proxy, idx, mapped_node, cons_fn); - - auto elm_holder = findElmHolder(untyped_proxy); - auto raw_ptr = elm_holder->lookup(idx).getRawPtr(); - raw_ptr->getLBData().updatePhase(thePhase()->getCurrentPhase()); } else if (insert_node != this_node) { auto msg = makeMessage>( proxy, idx, insert_node, mapped_node, modify_epoch, insert_msg diff --git a/tests/unit/collection/test_lb_data_retention.cc b/tests/unit/collection/test_lb_data_retention.cc index c02bd745c4..432e36fd9f 100644 --- a/tests/unit/collection/test_lb_data_retention.cc +++ b/tests/unit/collection/test_lb_data_retention.cc @@ -47,6 +47,7 @@ #include #include #include +#include #include @@ -58,7 +59,7 @@ namespace vt { namespace tests { namespace unit { struct TestCol : vt::Collection { - unsigned int prev_calls_ = 0; + unsigned int prev_calls_ = thePhase()->getCurrentPhase(); unsigned int prevCalls() { return prev_calls_++; } From 91cbe7c6bedb0ef59c790acd8c0bffc00c23dbab Mon Sep 17 00:00:00 2001 From: Arkadiusz Szczepkowicz Date: Tue, 10 Oct 2023 16:19:06 +0200 Subject: [PATCH 3/8] #1668: Add setPhase method to ElementLBData --- src/vt/elm/elm_lb_data.cc | 23 ++++++++++++----------- src/vt/elm/elm_lb_data.h | 1 + 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/vt/elm/elm_lb_data.cc b/src/vt/elm/elm_lb_data.cc index ef061c407d..6df48f9b0b 100644 --- a/src/vt/elm/elm_lb_data.cc +++ b/src/vt/elm/elm_lb_data.cc @@ -138,6 +138,17 @@ void ElementLBData::addTime(LoadType const timeLoad) { ); } +void ElementLBData::setPhase(PhaseType const& new_phase) { + cur_phase_ = new_phase; + + // Access all table entries for current phase, to ensure presence even + // if they're left empty + phase_timings_[cur_phase_]; + subphase_timings_[cur_phase_]; + phase_comm_[cur_phase_]; + subphase_comm_[cur_phase_]; +} + void ElementLBData::updatePhase(PhaseType const& inc) { vt_debug_print( verbose, lb, @@ -145,17 +156,7 @@ void ElementLBData::updatePhase(PhaseType const& inc) { cur_phase_, inc ); - cur_phase_ += inc; - - // Access all table entries for current phase, to ensure presence even - // if they're left empty - if ( cur_phase_ != 0 ) - { - phase_timings_[cur_phase_]; - subphase_timings_[cur_phase_]; - phase_comm_[cur_phase_]; - subphase_comm_[cur_phase_]; - } + setPhase(cur_phase_ + inc); } void ElementLBData::resetPhase() { diff --git a/src/vt/elm/elm_lb_data.h b/src/vt/elm/elm_lb_data.h index de96110b80..1ae770e484 100644 --- a/src/vt/elm/elm_lb_data.h +++ b/src/vt/elm/elm_lb_data.h @@ -81,6 +81,7 @@ struct ElementLBData { NodeType to, ElementIDStruct from_perm, double bytes, bool bcast ); + void setPhase(PhaseType const& new_phase); void updatePhase(PhaseType const& inc = 1); void resetPhase(); PhaseType getPhase() const; From ea34f154eb38e03e2812a5a777c8858cfbb7bf91 Mon Sep 17 00:00:00 2001 From: Arkadiusz Szczepkowicz Date: Tue, 10 Oct 2023 16:19:42 +0200 Subject: [PATCH 4/8] #1668: Use new setPhase instead of updatePhase --- src/vt/vrt/collection/manager.impl.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/vt/vrt/collection/manager.impl.h b/src/vt/vrt/collection/manager.impl.h index 5cfbd5ea8d..e8f4ecb8ca 100644 --- a/src/vt/vrt/collection/manager.impl.h +++ b/src/vt/vrt/collection/manager.impl.h @@ -1182,8 +1182,7 @@ bool CollectionManager::insertCollectionElement( std::move(vc) }); auto raw_ptr = elm_holder->lookup(idx).getRawPtr(); - raw_ptr->getLBData().resetPhase(); - raw_ptr->getLBData().updatePhase(thePhase()->getCurrentPhase()); + raw_ptr->getLBData().setPhase(thePhase()->getCurrentPhase()); if (is_migrated_in) { theLocMan()->getCollectionLM(proxy)->entityImmigrated( From 8a053c78536b31712c8ebb2e230cbb6c013ee53e Mon Sep 17 00:00:00 2001 From: Arkadiusz Szczepkowicz Date: Fri, 13 Oct 2023 13:35:21 +0200 Subject: [PATCH 5/8] #1668: Fix phase retention tests --- src/vt/elm/elm_lb_data.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/vt/elm/elm_lb_data.cc b/src/vt/elm/elm_lb_data.cc index 6df48f9b0b..2378f20e3a 100644 --- a/src/vt/elm/elm_lb_data.cc +++ b/src/vt/elm/elm_lb_data.cc @@ -143,10 +143,13 @@ void ElementLBData::setPhase(PhaseType const& new_phase) { // Access all table entries for current phase, to ensure presence even // if they're left empty - phase_timings_[cur_phase_]; - subphase_timings_[cur_phase_]; - phase_comm_[cur_phase_]; - subphase_comm_[cur_phase_]; + if ( cur_phase_ != 0 ) + { + phase_timings_[cur_phase_]; + subphase_timings_[cur_phase_]; + phase_comm_[cur_phase_]; + subphase_comm_[cur_phase_]; + } } void ElementLBData::updatePhase(PhaseType const& inc) { From 435156cba425062ec81ed039172419c991fec254 Mon Sep 17 00:00:00 2001 From: Arkadiusz Szczepkowicz Date: Fri, 13 Oct 2023 13:37:20 +0200 Subject: [PATCH 6/8] #1668: Remove whitespace --- src/vt/elm/elm_lb_data.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vt/elm/elm_lb_data.cc b/src/vt/elm/elm_lb_data.cc index 2378f20e3a..a15ee4d3eb 100644 --- a/src/vt/elm/elm_lb_data.cc +++ b/src/vt/elm/elm_lb_data.cc @@ -143,7 +143,7 @@ void ElementLBData::setPhase(PhaseType const& new_phase) { // Access all table entries for current phase, to ensure presence even // if they're left empty - if ( cur_phase_ != 0 ) + if ( cur_phase_ != 0 ) { phase_timings_[cur_phase_]; subphase_timings_[cur_phase_]; From 52ea1615cb800edf62ffae3602a5af4639aa2dd8 Mon Sep 17 00:00:00 2001 From: Arkadiusz Szczepkowicz Date: Fri, 13 Oct 2023 15:10:12 +0200 Subject: [PATCH 7/8] #1668: Fix some of the CI issue in tests --- src/vt/vrt/collection/manager.impl.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/vt/vrt/collection/manager.impl.h b/src/vt/vrt/collection/manager.impl.h index e8f4ecb8ca..4a4efa8ff1 100644 --- a/src/vt/vrt/collection/manager.impl.h +++ b/src/vt/vrt/collection/manager.impl.h @@ -1683,6 +1683,10 @@ void CollectionManager::insert( if (insert_node == this_node and proceed_with_insertion) { auto cons_fn = detail::InsertMsgDispatcher::makeCons(insert_msg); makeCollectionElement(untyped_proxy, idx, mapped_node, cons_fn); + + auto elm_holder = findElmHolder(untyped_proxy); + auto raw_ptr = elm_holder->lookup(idx).getRawPtr(); + raw_ptr->getLBData().updatePhase(thePhase()->getCurrentPhase()); } else if (insert_node != this_node) { auto msg = makeMessage>( proxy, idx, insert_node, mapped_node, modify_epoch, insert_msg From 2b39b4ac339b80312ae6f4a43e3727fac3354759 Mon Sep 17 00:00:00 2001 From: Arkadiusz Szczepkowicz Date: Wed, 18 Oct 2023 15:03:06 +0200 Subject: [PATCH 8/8] #1668: Move setPhase method call --- src/vt/vrt/collection/manager.impl.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/vt/vrt/collection/manager.impl.h b/src/vt/vrt/collection/manager.impl.h index 4a4efa8ff1..65679531a3 100644 --- a/src/vt/vrt/collection/manager.impl.h +++ b/src/vt/vrt/collection/manager.impl.h @@ -1181,8 +1181,6 @@ bool CollectionManager::insertCollectionElement( elm_holder->insert(idx, typename Holder::InnerHolder{ std::move(vc) }); - auto raw_ptr = elm_holder->lookup(idx).getRawPtr(); - raw_ptr->getLBData().setPhase(thePhase()->getCurrentPhase()); if (is_migrated_in) { theLocMan()->getCollectionLM(proxy)->entityImmigrated( @@ -1193,6 +1191,9 @@ bool CollectionManager::insertCollectionElement( listener::ElementEventEnum::ElementMigratedIn, idx, home_node ); } else { + auto raw_ptr = elm_holder->lookup(idx).getRawPtr(); + raw_ptr->getLBData().setPhase(thePhase()->getCurrentPhase()); + theLocMan()->getCollectionLM(proxy)->registerEntity( idx, home_node, CollectionManager::collectionMsgHandler @@ -1683,10 +1684,6 @@ void CollectionManager::insert( if (insert_node == this_node and proceed_with_insertion) { auto cons_fn = detail::InsertMsgDispatcher::makeCons(insert_msg); makeCollectionElement(untyped_proxy, idx, mapped_node, cons_fn); - - auto elm_holder = findElmHolder(untyped_proxy); - auto raw_ptr = elm_holder->lookup(idx).getRawPtr(); - raw_ptr->getLBData().updatePhase(thePhase()->getCurrentPhase()); } else if (insert_node != this_node) { auto msg = makeMessage>( proxy, idx, insert_node, mapped_node, modify_epoch, insert_msg