From 6b1498077aa712aace30c126a4551b4da7e0c786 Mon Sep 17 00:00:00 2001 From: JOAN VINYALS YLLA CATALA Date: Tue, 22 Oct 2024 11:47:23 +0200 Subject: [PATCH 1/6] Add a region summary in the detection backend Counting every time a region is open/closed and if any region is overlapped. And then report if which regions overlap and if times started and stopped match. --- src/backends/detection/detection.cpp | 88 +++++++++++++++++++++++++--- src/backends/detection/detection.hpp | 9 +++ 2 files changed, 88 insertions(+), 9 deletions(-) diff --git a/src/backends/detection/detection.cpp b/src/backends/detection/detection.cpp index 2be1c92..e405d97 100644 --- a/src/backends/detection/detection.cpp +++ b/src/backends/detection/detection.cpp @@ -71,6 +71,13 @@ void DetectionStrategy::RegionStart( start_entry.stack_ = region.stack_; start_entry.thread_ = getThreadStoreOfCaller(); + // Count region started + std::string region_name(region.name); + if (count_starts_.find(region_name) == count_starts_.end()) { + count_starts_[region_name] = 0; + } + count_starts_[region_name]++; + std::lock_guard guard(starts_mutex_); starts_.push_back(start_entry); } @@ -85,6 +92,13 @@ void DetectionStrategy::RegionStopLast( stop_entry.stack_ = region.stack_; stop_entry.thread_ = getThreadStoreOfCaller(); + // Count region closed + std::string region_name(region.name); + if (count_ends_.find(region_name) == count_ends_.end()) { + count_ends_[region_name] = 0; + } + count_ends_[region_name]++; + std::lock_guard guard(ends_mutex_); ends_.push_back(stop_entry); @@ -99,15 +113,27 @@ void DetectionStrategy::RegionStopLast( has_errors_ = true; return; } - // now check if we actually pop the right name - if (region.name.compare(region.stack_.top()) != 0) { - if (verbose_mode_.getValue().value_or(false)) { - std::cout << "neSmiK WARNING: The stack says the next region to close is " - << region.stack_.top() << " but tried closing " << region.name - << std::endl; - } - has_errors_ = true; - return; + + // now check if we actually pop the right name and if not we mark all the + // regions (until we find it) as overlapping. + bool is_overlapped = false; + std::stack stack = region.stack_; + while (region.name.compare(stack.top()) != 0) { + has_errors_ = true; + is_overlapped = true; + + overlapping_regions_.insert(std::string(stack.top())); + stack.pop(); + } + + if (is_overlapped) { + overlapping_regions_.insert(std::string(region.name)); + } + + if (is_overlapped && verbose_mode_.getValue().value_or(false)) { + std::cout << "neSmiK WARNING: The stack says the next region to close is " + << region.stack_.top() << " but tried closing " << region.name + << std::endl; } } @@ -149,6 +175,50 @@ void DetectionStrategy::Finalize() noexcept { } #endif + int comm_size = mpi_helper_.getCommSize(); + for (int i = 0; i < comm_size; i++) { + if (mpi_helper_.IsRankNumber(i)) { + // Print header + std::cout << "Process " << i << std::endl; + std::cout << "-----------------------------------------------------"; + if (!overlapped_regions_.empty()) { + std::cout << "---------------"; + } + std::cout << std::endl; + + // Print regions + for (auto starts : count_starts_) { + auto ends = count_ends_.extract(starts.first); + std::cout << std::setw(30) << starts.first << ": "; + std::cout << std::setw(10) << starts.second; + std::cout << "/"; + std::cout << std::setw(10) << ends.mapped(); + if (overlapped_regions_.contains(starts.first)) { + std::cout << " (overlapping) "; + } + std::cout << std::endl; + } + for (auto ends : count_ends_) { + std::cout << std::setw(30) << ends.first << ": "; + std::cout << std::setw(10) << 0; + std::cout << "/"; + std::cout << std::setw(10) << ends.second; + if (overlapped_regions_.contains(ends.first)) { + std::cout << " (overlapping) "; + } + std::cout << std::endl; + } + std::cout << "-----------------------------------------------------"; + if (!overlapped_regions_.empty()) { + std::cout << "---------------"; + } + std::cout << std::endl; + } +#ifdef WITH_MPI + PMPI_Barrier(MPI_COMM_WORLD); +#endif + } + // print summary report if (mpi_helper_.IsRankNumber(0)) { if (mpi_helper_.IsUsingMPI()) { diff --git a/src/backends/detection/detection.hpp b/src/backends/detection/detection.hpp index ed7f499..bb03b02 100644 --- a/src/backends/detection/detection.hpp +++ b/src/backends/detection/detection.hpp @@ -11,6 +11,8 @@ #include #include #include +#include +#include // for convenience using json = nlohmann::json; @@ -36,6 +38,13 @@ class DetectionStrategy : public ProperlyNestedAnnotationStrategy { std::vector starts_; std::vector ends_; + // Regions that were overlapped or overlapping at some point + std::set overlapping_regions_; + + // Count of times a regions was open/closed + std::unordered_map count_starts_; + std::unordered_map count_ends_; + std::mutex starts_mutex_; std::mutex ends_mutex_; EnvironmentVariable verbose_mode_ = EnvironmentVariable( -- GitLab From 2bd799cd48c03a720d7c88f5bd3a3f27516075e1 Mon Sep 17 00:00:00 2001 From: JOAN VINYALS YLLA CATALA Date: Tue, 22 Oct 2024 11:49:57 +0200 Subject: [PATCH 2/6] clang-format --- src/backends/detection/detection.cpp | 88 ++++++++++++++-------------- src/backends/detection/detection.hpp | 4 +- 2 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/backends/detection/detection.cpp b/src/backends/detection/detection.cpp index e405d97..e0dea68 100644 --- a/src/backends/detection/detection.cpp +++ b/src/backends/detection/detection.cpp @@ -74,7 +74,7 @@ void DetectionStrategy::RegionStart( // Count region started std::string region_name(region.name); if (count_starts_.find(region_name) == count_starts_.end()) { - count_starts_[region_name] = 0; + count_starts_[region_name] = 0; } count_starts_[region_name]++; @@ -95,7 +95,7 @@ void DetectionStrategy::RegionStopLast( // Count region closed std::string region_name(region.name); if (count_ends_.find(region_name) == count_ends_.end()) { - count_ends_[region_name] = 0; + count_ends_[region_name] = 0; } count_ends_[region_name]++; @@ -119,15 +119,15 @@ void DetectionStrategy::RegionStopLast( bool is_overlapped = false; std::stack stack = region.stack_; while (region.name.compare(stack.top()) != 0) { - has_errors_ = true; - is_overlapped = true; + has_errors_ = true; + is_overlapped = true; - overlapping_regions_.insert(std::string(stack.top())); - stack.pop(); + overlapping_regions_.insert(std::string(stack.top())); + stack.pop(); } if (is_overlapped) { - overlapping_regions_.insert(std::string(region.name)); + overlapping_regions_.insert(std::string(region.name)); } if (is_overlapped && verbose_mode_.getValue().value_or(false)) { @@ -177,45 +177,45 @@ void DetectionStrategy::Finalize() noexcept { int comm_size = mpi_helper_.getCommSize(); for (int i = 0; i < comm_size; i++) { - if (mpi_helper_.IsRankNumber(i)) { - // Print header - std::cout << "Process " << i << std::endl; - std::cout << "-----------------------------------------------------"; - if (!overlapped_regions_.empty()) { - std::cout << "---------------"; - } - std::cout << std::endl; - - // Print regions - for (auto starts : count_starts_) { - auto ends = count_ends_.extract(starts.first); - std::cout << std::setw(30) << starts.first << ": "; - std::cout << std::setw(10) << starts.second; - std::cout << "/"; - std::cout << std::setw(10) << ends.mapped(); - if (overlapped_regions_.contains(starts.first)) { - std::cout << " (overlapping) "; - } - std::cout << std::endl; - } - for (auto ends : count_ends_) { - std::cout << std::setw(30) << ends.first << ": "; - std::cout << std::setw(10) << 0; - std::cout << "/"; - std::cout << std::setw(10) << ends.second; - if (overlapped_regions_.contains(ends.first)) { - std::cout << " (overlapping) "; - } - std::cout << std::endl; - } - std::cout << "-----------------------------------------------------"; - if (!overlapped_regions_.empty()) { - std::cout << "---------------"; - } - std::cout << std::endl; + if (mpi_helper_.IsRankNumber(i)) { + // Print header + std::cout << "Process " << i << std::endl; + std::cout << "-----------------------------------------------------"; + if (!overlapped_regions_.empty()) { + std::cout << "---------------"; + } + std::cout << std::endl; + + // Print regions + for (auto starts : count_starts_) { + auto ends = count_ends_.extract(starts.first); + std::cout << std::setw(30) << starts.first << ": "; + std::cout << std::setw(10) << starts.second; + std::cout << "/"; + std::cout << std::setw(10) << ends.mapped(); + if (overlapped_regions_.contains(starts.first)) { + std::cout << " (overlapping) "; + } + std::cout << std::endl; + } + for (auto ends : count_ends_) { + std::cout << std::setw(30) << ends.first << ": "; + std::cout << std::setw(10) << 0; + std::cout << "/"; + std::cout << std::setw(10) << ends.second; + if (overlapped_regions_.contains(ends.first)) { + std::cout << " (overlapping) "; + } + std::cout << std::endl; } + std::cout << "-----------------------------------------------------"; + if (!overlapped_regions_.empty()) { + std::cout << "---------------"; + } + std::cout << std::endl; + } #ifdef WITH_MPI - PMPI_Barrier(MPI_COMM_WORLD); + PMPI_Barrier(MPI_COMM_WORLD); #endif } diff --git a/src/backends/detection/detection.hpp b/src/backends/detection/detection.hpp index bb03b02..af67f93 100644 --- a/src/backends/detection/detection.hpp +++ b/src/backends/detection/detection.hpp @@ -6,13 +6,13 @@ #include #include +#include #include #include +#include #include #include #include -#include -#include // for convenience using json = nlohmann::json; -- GitLab From 713d3b20a7a984a7c9a3677e2d21860956fd142f Mon Sep 17 00:00:00 2001 From: JOAN VINYALS YLLA CATALA Date: Tue, 22 Oct 2024 11:56:42 +0200 Subject: [PATCH 3/6] Fix compiling errors --- src/backends/detection/detection.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/backends/detection/detection.cpp b/src/backends/detection/detection.cpp index e0dea68..fea4a4c 100644 --- a/src/backends/detection/detection.cpp +++ b/src/backends/detection/detection.cpp @@ -181,7 +181,7 @@ void DetectionStrategy::Finalize() noexcept { // Print header std::cout << "Process " << i << std::endl; std::cout << "-----------------------------------------------------"; - if (!overlapped_regions_.empty()) { + if (!overlapping_regions_.empty()) { std::cout << "---------------"; } std::cout << std::endl; @@ -193,7 +193,7 @@ void DetectionStrategy::Finalize() noexcept { std::cout << std::setw(10) << starts.second; std::cout << "/"; std::cout << std::setw(10) << ends.mapped(); - if (overlapped_regions_.contains(starts.first)) { + if (overlapping_regions_.count(starts.first) > 0) { std::cout << " (overlapping) "; } std::cout << std::endl; @@ -203,13 +203,13 @@ void DetectionStrategy::Finalize() noexcept { std::cout << std::setw(10) << 0; std::cout << "/"; std::cout << std::setw(10) << ends.second; - if (overlapped_regions_.contains(ends.first)) { + if (overlapping_regions_.count(ends.first) > 0) { std::cout << " (overlapping) "; } std::cout << std::endl; } std::cout << "-----------------------------------------------------"; - if (!overlapped_regions_.empty()) { + if (!overlapping_regions_.empty()) { std::cout << "---------------"; } std::cout << std::endl; -- GitLab From 4fb26921cbfc70690fa34e9a7b58743fb6318a71 Mon Sep 17 00:00:00 2001 From: JOAN VINYALS YLLA CATALA Date: Tue, 22 Oct 2024 13:20:28 +0200 Subject: [PATCH 4/6] Make it work for non-MPI --- src/backends/detection/detection.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/backends/detection/detection.cpp b/src/backends/detection/detection.cpp index fea4a4c..b155e12 100644 --- a/src/backends/detection/detection.cpp +++ b/src/backends/detection/detection.cpp @@ -176,13 +176,20 @@ void DetectionStrategy::Finalize() noexcept { #endif int comm_size = mpi_helper_.getCommSize(); + + // TODO: Fix the Parallelism Helper as it should report at comm_size = 1 when + // there is no MPI present to make MPI aware backends to work, without much + // spegetty code. + if (comm_size == 0) { + comm_size++; + } for (int i = 0; i < comm_size; i++) { if (mpi_helper_.IsRankNumber(i)) { // Print header std::cout << "Process " << i << std::endl; - std::cout << "-----------------------------------------------------"; + std::cout << "------------------------------------------------------"; if (!overlapping_regions_.empty()) { - std::cout << "---------------"; + std::cout << "--------------"; } std::cout << std::endl; @@ -208,9 +215,9 @@ void DetectionStrategy::Finalize() noexcept { } std::cout << std::endl; } - std::cout << "-----------------------------------------------------"; + std::cout << "------------------------------------------------------"; if (!overlapping_regions_.empty()) { - std::cout << "---------------"; + std::cout << "--------------"; } std::cout << std::endl; } -- GitLab From bb1bf163e843607b1f377620911966731e360c81 Mon Sep 17 00:00:00 2001 From: JOAN VINYALS YLLA CATALA Date: Mon, 28 Oct 2024 22:02:12 +0100 Subject: [PATCH 5/6] feat: Improve region summary report structure and content Refactor how region summaries are generated and displayed to include more detail about starts, stops, overlapping status, and thread safety. Ensure that regions are only added to the report if requested, if there is a mismatch in starts/stops, if the region overlaps, or if it is not thread-safe. Unless the user sets a variable to show all regions in the report. --- src/backends/detection/detection.cpp | 122 ++++++++++++++++++++++----- src/delegator.cpp | 5 +- src/delegator.hpp | 4 + tests/cpp/TestCppNotNested.cpp | 24 ++++++ 4 files changed, 133 insertions(+), 22 deletions(-) create mode 100644 tests/cpp/TestCppNotNested.cpp diff --git a/src/backends/detection/detection.cpp b/src/backends/detection/detection.cpp index b155e12..d1da779 100644 --- a/src/backends/detection/detection.cpp +++ b/src/backends/detection/detection.cpp @@ -118,12 +118,19 @@ void DetectionStrategy::RegionStopLast( // regions (until we find it) as overlapping. bool is_overlapped = false; std::stack stack = region.stack_; - while (region.name.compare(stack.top()) != 0) { + while (!stack.empty() && region.name.compare(stack.top()) != 0) { + // Get value and pop + std::string top_region = stack.top(); + stack.pop(); + + if (overlapping_regions_.count(top_region) > 0) { + break; + } + has_errors_ = true; is_overlapped = true; - overlapping_regions_.insert(std::string(stack.top())); - stack.pop(); + overlapping_regions_.insert(std::string(top_region)); } if (is_overlapped) { @@ -137,18 +144,28 @@ void DetectionStrategy::RegionStopLast( } } +typedef struct region_summary_t { + unsigned int starts; + unsigned int stops; + bool overlapping; + bool thread_safe; +} region_summary_t; + void DetectionStrategy::Finalize() noexcept { int has_errors = static_cast(has_errors_); int has_been_called_from_threads = 0; pid_t first_thread_id = 0; + std::set used_multiple_threads; + for (const auto& start : starts_) { if (first_thread_id == 0) { first_thread_id = start.thread_.thread_id_; } if (start.thread_.thread_id_ != first_thread_id) { has_been_called_from_threads = 1; + used_multiple_threads.insert(std::string(start.region_name_)); break; } } @@ -159,6 +176,7 @@ void DetectionStrategy::Finalize() noexcept { } if (end.thread_.thread_id_ != first_thread_id) { has_been_called_from_threads = 1; + used_multiple_threads.insert(std::string(end.region_name_)); break; } } @@ -183,42 +201,106 @@ void DetectionStrategy::Finalize() noexcept { if (comm_size == 0) { comm_size++; } + + bool someone_is_overlapping = overlapping_regions_.size() > 0; + bool someone_is_not_thread_safe = used_multiple_threads.size() > 0; + std::unordered_map region_summaries; + for (int i = 0; i < comm_size; i++) { if (mpi_helper_.IsRankNumber(i)) { + // Preparing region summary entries + // -------------------------------- + for (auto starts : count_starts_) { + unsigned int counted_ends = 0; + if (count_ends_.find(starts.first) != count_ends_.end()) { + auto ends = count_ends_.extract(starts.first); + counted_ends = ends.mapped(); + } + + bool overlapping = overlapping_regions_.count(starts.first) > 0; + bool thread_safe = used_multiple_threads.count(starts.first) == 0; + used_multiple_threads.erase(starts.first); + + bool mismatched = starts.second != counted_ends; + + // We add the report in the list either if we: + // - print everything as requested by the user + // - find that there is a mismatch between starts and stops + // - find that the region is overlapping with another + // - find that the region is not thread safe + if (show_all_regions_.getValue().value_or(false) || mismatched || + overlapping || !thread_safe) { + // We assume regions won't be repeated + region_summaries[starts.first] = { + .starts = starts.second, + .stops = counted_ends, + .overlapping = overlapping, + .thread_safe = thread_safe, + }; + } + } + for (auto ends : count_ends_) { + bool overlapping = overlapping_regions_.count(ends.first) > 0; + bool thread_safe = used_multiple_threads.count(ends.first) > 0; + used_multiple_threads.erase(ends.first); + + // All regions that make it that far must be printed because will have + // never been started. + region_summaries[ends.first] = { + .starts = 0, + .stops = ends.second, + .overlapping = overlapping, + .thread_safe = thread_safe, + }; + } + // The list of used multiple threads should be empty as all regions + // should have been treated by this point + // + // assert(used_multiple_threads.empty()) + + // Printing the summary + // -------------------- + // // Print header std::cout << "Process " << i << std::endl; std::cout << "------------------------------------------------------"; - if (!overlapping_regions_.empty()) { + if (someone_is_overlapping) { std::cout << "--------------"; } + if (someone_is_not_thread_safe) { + std::cout << "---------------"; + } std::cout << std::endl; // Print regions - for (auto starts : count_starts_) { - auto ends = count_ends_.extract(starts.first); - std::cout << std::setw(30) << starts.first << ": "; - std::cout << std::setw(10) << starts.second; + for (auto region_summary : region_summaries) { + std::cout << std::setw(30) << region_summary.first << ": "; + std::cout << std::setw(10) << region_summary.second.starts; std::cout << "/"; - std::cout << std::setw(10) << ends.mapped(); - if (overlapping_regions_.count(starts.first) > 0) { + std::cout << std::setw(10) << region_summary.second.stops; + + if (region_summary.second.overlapping) { std::cout << " (overlapping) "; + } else if (someone_is_overlapping) { + std::cout << " "; } - std::cout << std::endl; - } - for (auto ends : count_ends_) { - std::cout << std::setw(30) << ends.first << ": "; - std::cout << std::setw(10) << 0; - std::cout << "/"; - std::cout << std::setw(10) << ends.second; - if (overlapping_regions_.count(ends.first) > 0) { - std::cout << " (overlapping) "; + + if (!region_summary.second.thread_safe) { + std::cout << " (multithread) "; + } else if (someone_is_not_thread_safe) { + std::cout << " "; } std::cout << std::endl; } + + // Close process summary std::cout << "------------------------------------------------------"; - if (!overlapping_regions_.empty()) { + if (someone_is_overlapping) { std::cout << "--------------"; } + if (someone_is_not_thread_safe) { + std::cout << "---------------"; + } std::cout << std::endl; } #ifdef WITH_MPI diff --git a/src/delegator.cpp b/src/delegator.cpp index d9e8e9c..21d7d4d 100644 --- a/src/delegator.cpp +++ b/src/delegator.cpp @@ -234,9 +234,10 @@ void Delegator::RegionStop(std::string_view name) { break; } if (!name_stack_.empty()) { - if (name.compare(name_stack_.top()) != 0) { + if (name.compare(name_stack_.top()) != 0 && + verbose_mode_.getValue().value_or(false)) { std::cout << "neSmiK region_close() expected " << name_stack_.top() - << "but got " << name << std::endl; + << " but got " << name << std::endl; } name_stack_.pop(); } diff --git a/src/delegator.hpp b/src/delegator.hpp index 57ab41d..cfb40ca 100644 --- a/src/delegator.hpp +++ b/src/delegator.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -32,6 +33,9 @@ class Delegator { inline static std::unique_ptr mpi_helper_; + inline static EnvironmentVariable verbose_mode_ = + EnvironmentVariable("VERBOSE", "Enables the verbose mode", false); + public: static void Init(); static void RegionStart(std::string_view name); diff --git a/tests/cpp/TestCppNotNested.cpp b/tests/cpp/TestCppNotNested.cpp new file mode 100644 index 0000000..0200d2e --- /dev/null +++ b/tests/cpp/TestCppNotNested.cpp @@ -0,0 +1,24 @@ +#include "nesmik/nesmik.hpp" + +int main() { + nesmik::nesmik_init(); + nesmik::region_start("Level 1"); + nesmik::region_start("Level 2"); + nesmik::region_start("Overlapped 1"); + nesmik::region_start("Overlapped 2"); + nesmik::region_start("Level 3"); + nesmik::region_stop("Level 3"); + nesmik::region_stop("Overlapped 1"); + nesmik::region_stop("Overlapped 2"); + +#pragma omp parallel num_threads(2) + { + nesmik::region_start("Multithread"); + nesmik::region_stop("Multithread"); + } + + nesmik::region_stop("Level 2"); + nesmik::region_stop("Level 1"); + nesmik::region_start("Never closed"); + nesmik::nesmik_finalize(); +} -- GitLab From 2072650dc3774910e38b5ce7d0d2085ca19ccc5d Mon Sep 17 00:00:00 2001 From: JOAN VINYALS YLLA CATALA Date: Mon, 28 Oct 2024 22:15:34 +0100 Subject: [PATCH 6/6] feat: Add show all regions option and adjust detection loop The changes add a show all regions option and adjust the detection loop to handle overlapping regions properly. build: Add option to show summary for all regions --- src/backends/detection/detection.cpp | 2 +- src/backends/detection/detection.hpp | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/backends/detection/detection.cpp b/src/backends/detection/detection.cpp index d1da779..39c3baf 100644 --- a/src/backends/detection/detection.cpp +++ b/src/backends/detection/detection.cpp @@ -199,7 +199,7 @@ void DetectionStrategy::Finalize() noexcept { // there is no MPI present to make MPI aware backends to work, without much // spegetty code. if (comm_size == 0) { - comm_size++; + comm_size++; } bool someone_is_overlapping = overlapping_regions_.size() > 0; diff --git a/src/backends/detection/detection.hpp b/src/backends/detection/detection.hpp index af67f93..dfd5b4e 100644 --- a/src/backends/detection/detection.hpp +++ b/src/backends/detection/detection.hpp @@ -51,6 +51,12 @@ class DetectionStrategy : public ProperlyNestedAnnotationStrategy { "DETECTION_VERBOSE", "Enables the verbose mode in the detection backend", false); + EnvironmentVariable show_all_regions_ = EnvironmentVariable( + "SHOW_ALL_REGIONS", + "Report summary for all regions. If disabled, only regions with odd " + "behaviour will be reported. (default:off)", + false); + bool has_errors_ = false; public: -- GitLab