From 810442fdc8155676cad2434a433eabf7dcbbc957 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Steffan=20S=C3=B8lvsten?= Date: Mon, 1 Jul 2024 13:49:23 +0200 Subject: [PATCH 1/3] Update comments in to 100 --- src/adiar/internal/data_structures/sorter.h | 87 ++++++++++----------- 1 file changed, 42 insertions(+), 45 deletions(-) diff --git a/src/adiar/internal/data_structures/sorter.h b/src/adiar/internal/data_structures/sorter.h index 6bc845af7..5a4c8833e 100644 --- a/src/adiar/internal/data_structures/sorter.h +++ b/src/adiar/internal/data_structures/sorter.h @@ -17,9 +17,9 @@ namespace adiar::internal template > class sorter; - ////////////////////////////////////////////////////////////////////////////// + ////////////////////////////////////////////////////////////////////////////////////////////////// /// \brief Wrapper for TPIE's internal vector with standard quick-sort. - ////////////////////////////////////////////////////////////////////////////// + ////////////////////////////////////////////////////////////////////////////////////////////////// template class sorter { @@ -144,23 +144,21 @@ namespace adiar::internal } }; - ////////////////////////////////////////////////////////////////////////////// - /// \brief Type alias for sorter for partial type application of the - /// 'internal' memory type. - ////////////////////////////////////////////////////////////////////////////// + ////////////////////////////////////////////////////////////////////////////////////////////////// + /// \brief Type alias for sorter for partial type application of the 'internal' memory type. + ////////////////////////////////////////////////////////////////////////////////////////////////// template > using internal_sorter = sorter; // LCOV_EXCL_START // TODO: Unit test external memory variants? - ////////////////////////////////////////////////////////////////////////////// + ////////////////////////////////////////////////////////////////////////////////////////////////// /// \brief Wrapper for TPIE's external memory sorter, tpie::merge_sorter. /// - /// A wrapper for the tpie::merge_sorter that takes care of all the memory - /// computations involved in deriving how much memory should be used and how - /// it should be. - ////////////////////////////////////////////////////////////////////////////// + /// A wrapper for the tpie::merge_sorter that takes care of all the memory computations involved + /// in deriving how much memory should be used and how it should be. + ////////////////////////////////////////////////////////////////////////////////////////////////// template class sorter { @@ -199,7 +197,7 @@ namespace adiar::internal sorter(size_t memory_bytes, size_t no_elements, size_t number_of_sorters, Comp comp = Comp()) : _sorter(comp) { - // ======================================================================= + // =========================================================================================== // Case 0: No sorters - why are we then instantiating one? adiar_assert(number_of_sorters > 0, "Number of sorters should be positive"); @@ -209,7 +207,7 @@ namespace adiar::internal const tpie::memory_size_type no_elements_memory = 2 * sorter::memory_usage(no_elements); - // ======================================================================= + // =========================================================================================== // Case 1: A single sorter. if (number_of_sorters == 1u) { const size_t sorter_memory = std::min(no_elements_memory, memory_bytes); @@ -217,45 +215,45 @@ namespace adiar::internal adiar_assert(sorter_memory <= memory_bytes, "Memory of a single sorter does not exceed given amount."); - // --------------------------------------------------------------------- + // ----------------------------------------------------------------------------------------- // Use TPIE's default settings for the three phases. _sorter.set_available_memory(sorter_memory); _sorter.begin(); return; } - // ====================================================================== + // ========================================================================================== // Case: 2+: Distribute a common area of memory between all sorters. + // + // Quickfix: Issue https://github.com/thomasmoelhave/tpie/issues/250 + constexpr tpie::memory_size_type minimum_phase1 = + sizeof(value_type) * 128 * 1024 + 5 * 1024 * 1024; + adiar_assert(number_of_sorters > 1, "Edge case for a single sorter taken care of above"); - // ----------------------------------------------------------------------- - // We intend to have the memory divided between all the sorters, such that - // one can be in phase 2 while everyone else is in phase 1 or 3. + // ------------------------------------------------------------------------------------------- + // We intend to have the memory divided between all the sorters, such that one can be in phase + // 2 while everyone else is in phase 1 or 3. // // | p1 | p1 | p1 | . . . p2 . . . | // - // Phase two is the one that makes the most out of a lot of memory. So, we - // want to have phase 2 have the largest share. For simplicity, we - // currently have all the p1 sorters have 1/16 of the entire memory. + // Phase two is the one that makes the most out of a lot of memory. So, we want to have phase + // 2 have the largest share. For simplicity, we currently have all the p1 sorters have 1/16 of + // the entire memory. // - // TODO: It would be better to make p2 'exponentially' larger than p1 for - // some notion of 'exponentiality'. + // TODO: It would be better to make p2 'exponentially' larger than p1 for some notion of + // 'exponentiality'. // - // TODO: phase 1 should be upper bounded by the number of elements - // possibly placed in this queue. See Issue #189 on ssoelvsten/adiar - // as to why. I would suggest for us to upper bound the 1/16th by - // slightly more than the amount of memory necessary to hold all + // TODO: phase 1 should be upper bounded by the number of elements possibly placed in this + // queue. See Issue #189 on ssoelvsten/adiar as to why. I would suggest for us to upper + // bound the 1/16th by slightly more than the amount of memory necessary to hold all // values simultaneously. - // ----------------------------------------------------------------------- + // ------------------------------------------------------------------------------------------- // Phase 1 : Push Mergesort base cases - // - // Quickfix: Issue https://github.com/thomasmoelhave/tpie/issues/250 - constexpr tpie::memory_size_type minimum_phase1 = - sizeof(value_type) * 128 * 1024 + 5 * 1024 * 1024; - // Take up at most 1/(Sorter-1)'th of 1/16th of the total memory. The last - // sorter is either in the same phase or another phase. + // Take up at most 1/(Sorter-1)'th of 1/16th of the total memory. The last sorter is either in + // the same phase or another phase. // // Intendeds shared memory layout of sorters // +--------+-----------------------------------------------------+ @@ -266,20 +264,20 @@ namespace adiar::internal const tpie::memory_size_type phase1 = std::max(minimum_phase1, std::min(maximum_phase1, no_elements_memory)); - // ----------------------------------------------------------------------- + // ------------------------------------------------------------------------------------------- // Phase 3 : Top-most and final merging of partial lists. // - // Based on internal workings of `tpie::merge_sorter` this should be at - // least twice the size of the phase 1 memory. + // Based on internal workings of `tpie::merge_sorter` this should be at least twice the size + // of the phase 1 memory. constexpr tpie::memory_size_type minimum_phase3 = /*2 **/ minimum_phase1; const tpie::memory_size_type phase3 = std::max(minimum_phase3, phase1); - // ----------------------------------------------------------------------- + // ------------------------------------------------------------------------------------------- // Phase 2 : Merge sorted lists until there are few enough for phase 3. // - // Use the remaining sorter memory for this very step. If one is in this - // phase, then all other sorters are in phase 1. + // Use the remaining sorter memory for this very step. If one is in this phase, then all other + // sorters are in phase 1. const tpie::memory_size_type phase2 = memory_bytes - phase1 * (number_of_sorters - 1); // ----------------------------------------------------------------------- @@ -296,7 +294,7 @@ namespace adiar::internal adiar_assert((number_of_sorters - 1) * phase1 + phase3 + phase1 <= memory_bytes, "Memory is enough to replace the pullable sorter with an 'Nth' pushable one."); - // ----------------------------------------------------------------------- + // ------------------------------------------------------------------------------------------- // Set the available memory and start the sorter _sorter.set_available_memory(phase1, phase2, phase3); _sorter.begin(); @@ -366,10 +364,9 @@ namespace adiar::internal } }; - ////////////////////////////////////////////////////////////////////////////// - /// \brief Type alias for sorter for partial type application of the - /// 'external' memory type. - ////////////////////////////////////////////////////////////////////////////// + ////////////////////////////////////////////////////////////////////////////////////////////////// + /// \brief Type alias for sorter for partial type application of the 'external' memory type. + ////////////////////////////////////////////////////////////////////////////////////////////////// template > using external_sorter = sorter; From 291d683ad6a8f655c83561f54db3a9c9b32c0f9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Steffan=20S=C3=B8lvsten?= Date: Mon, 1 Jul 2024 13:51:59 +0200 Subject: [PATCH 2/3] Fix TPIE (external) sorter does not have enough memory for 'number_of_sorters == 1' --- src/adiar/internal/data_structures/sorter.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/adiar/internal/data_structures/sorter.h b/src/adiar/internal/data_structures/sorter.h index 5a4c8833e..f4bc2cea1 100644 --- a/src/adiar/internal/data_structures/sorter.h +++ b/src/adiar/internal/data_structures/sorter.h @@ -197,6 +197,10 @@ namespace adiar::internal sorter(size_t memory_bytes, size_t no_elements, size_t number_of_sorters, Comp comp = Comp()) : _sorter(comp) { + // Quickfix: Issue https://github.com/thomasmoelhave/tpie/issues/250 + constexpr tpie::memory_size_type minimum_phase1 = + sizeof(value_type) * 128 * 1024 + 5 * 1024 * 1024; + // =========================================================================================== // Case 0: No sorters - why are we then instantiating one? adiar_assert(number_of_sorters > 0, "Number of sorters should be positive"); @@ -210,7 +214,8 @@ namespace adiar::internal // =========================================================================================== // Case 1: A single sorter. if (number_of_sorters == 1u) { - const size_t sorter_memory = std::min(no_elements_memory, memory_bytes); + const size_t sorter_memory = + std::min(std::max(no_elements_memory, 2 * minimum_phase1), memory_bytes); adiar_assert(sorter_memory <= memory_bytes, "Memory of a single sorter does not exceed given amount."); @@ -224,11 +229,6 @@ namespace adiar::internal // ========================================================================================== // Case: 2+: Distribute a common area of memory between all sorters. - // - // Quickfix: Issue https://github.com/thomasmoelhave/tpie/issues/250 - constexpr tpie::memory_size_type minimum_phase1 = - sizeof(value_type) * 128 * 1024 + 5 * 1024 * 1024; - adiar_assert(number_of_sorters > 1, "Edge case for a single sorter taken care of above"); // ------------------------------------------------------------------------------------------- From 721030996ead4f7b4121f85cd194217b9d656145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Steffan=20S=C3=B8lvsten?= Date: Mon, 1 Jul 2024 15:02:44 +0200 Subject: [PATCH 3/3] Fix warning on comparison of signed and unsigned (Thanks, Clang) --- src/adiar/internal/data_types/level_info.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/adiar/internal/data_types/level_info.h b/src/adiar/internal/data_types/level_info.h index 398c2387e..3dd6138f7 100644 --- a/src/adiar/internal/data_types/level_info.h +++ b/src/adiar/internal/data_types/level_info.h @@ -172,7 +172,7 @@ namespace adiar::internal using level_type = level_info::level_type; using signed_level_type = level_info::signed_level_type; - adiar_assert(levels < 0 ? (std::abs(levels) <= li.level()) + adiar_assert(levels < 0 ? (static_cast(std::abs(levels)) <= li.level()) : (li.level() + levels <= ptr_uint64::max_label)); // TODO: Use dynamic casts instead?