From f1f96166c5830412459720eec87589e6064dd744 Mon Sep 17 00:00:00 2001 From: Michael Bell Date: Tue, 21 Sep 2021 18:09:38 +0100 Subject: [PATCH] Fix MLD level mask generation to support 64-bit masks. (#6123) The generation of level masks for compactly storing partition cells supports sizes that can be stored in 64 bits. The current implementation fails if the total bit sum is 64 bits exactly. A bit shift mechanism is used that is undefined when the shift size is equal to the bit size of the underlying type. This generates an incorrect mask value. We fix this by adding a special case for a 64 bit offset. Given this code is called at most |level| times, there will be no effect on performance. We also update the assertions to reflect 64 bit masks are now supported. --- CHANGELOG.md | 1 + include/partitioner/multi_level_partition.hpp | 15 +++-- .../partitioner/multi_level_partition.cpp | 57 +++++++++++++++++++ 3 files changed, 68 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bcf36c0af3..c0038417bb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - FIXED: Fixed Boost link flags in pkg-config file. [#6083](https://github.com/Project-OSRM/osrm-backend/pull/6083) - Routing: - FIXED: Fix generation of inefficient MLD partitions [#6084](https://github.com/Project-OSRM/osrm-backend/pull/6084) + - FIXED: Fix MLD level mask generation to support 64-bit masks. [#6123](https://github.com/Project-OSRM/osrm-backend/pull/6123) # 5.25.0 - Changes from 5.24.0 diff --git a/include/partitioner/multi_level_partition.hpp b/include/partitioner/multi_level_partition.hpp index 855aa5ae2f0..d83f9a341aa 100644 --- a/include/partitioner/multi_level_partition.hpp +++ b/include/partitioner/multi_level_partition.hpp @@ -186,11 +186,12 @@ template class MultiLevelPartitionImpl final auto bits = static_cast(std::ceil(std::log2(num_cells + 1))); offsets[lidx++] = sum_bits; sum_bits += bits; - if (sum_bits > 64) + if (sum_bits > NUM_PARTITION_BITS) { throw util::exception( "Can't pack the partition information at level " + std::to_string(lidx) + - " into a 64bit integer. Would require " + std::to_string(sum_bits) + " bits."); + " into a " + std::to_string(NUM_PARTITION_BITS) + + "bit integer. Would require " + std::to_string(sum_bits) + " bits."); } } // sentinel @@ -211,11 +212,15 @@ template class MultiLevelPartitionImpl final [&](const auto offset, const auto next_offset) { // create mask that has `bits` ones at its LSBs. // 000011 - BOOST_ASSERT(offset < NUM_PARTITION_BITS); + BOOST_ASSERT(offset <= NUM_PARTITION_BITS); PartitionID mask = (1ULL << offset) - 1ULL; // 001111 - BOOST_ASSERT(next_offset < NUM_PARTITION_BITS); - PartitionID next_mask = (1ULL << next_offset) - 1ULL; + BOOST_ASSERT(next_offset <= NUM_PARTITION_BITS); + // Check offset for shift overflow. Offsets are strictly increasing, + // so we only need the check on the last mask. + PartitionID next_mask = next_offset == NUM_PARTITION_BITS + ? -1ULL + : (1ULL << next_offset) - 1ULL; // 001100 masks[lidx++] = next_mask ^ mask; }); diff --git a/unit_tests/partitioner/multi_level_partition.cpp b/unit_tests/partitioner/multi_level_partition.cpp index d25664525e9..7fc7acd1452 100644 --- a/unit_tests/partitioner/multi_level_partition.cpp +++ b/unit_tests/partitioner/multi_level_partition.cpp @@ -1,6 +1,8 @@ #include #include +#include "util/exception.hpp" +#include "util/for_each_indexed.hpp" #include #include @@ -230,4 +232,59 @@ BOOST_AUTO_TEST_CASE(large_cell_number) } } +BOOST_AUTO_TEST_CASE(cell_64_bits) +{ + // bits = ceil(log2(2458529 + 1)) + ceil(log2(258451 + 1)) + ceil(log2(16310 + 1)) + + // ceil(log2(534 + 1)) + // = 22 + 18 + 14 + 10 + // = 64 + const size_t NUM_PARTITIONS = 2458529; + const std::vector level_cells = {NUM_PARTITIONS, 258451, 16310, 534}; + std::vector> levels(level_cells.size(), + std::vector(level_cells[0])); + std::vector levels_to_num_cells(level_cells.size()); + + const auto set_level_cells = [&](size_t level, auto const num_cells) { + for (auto val : util::irange(0ULL, NUM_PARTITIONS)) + { + levels[level][val] = std::min(val, num_cells - 1); + } + levels_to_num_cells[level] = num_cells; + }; + util::for_each_indexed(level_cells.cbegin(), level_cells.cend(), set_level_cells); + + MultiLevelPartition mlp{levels, levels_to_num_cells}; + + BOOST_REQUIRE_EQUAL(mlp.GetNumberOfCells(1), level_cells[0]); + BOOST_REQUIRE_EQUAL(mlp.GetNumberOfCells(2), level_cells[1]); + BOOST_REQUIRE_EQUAL(mlp.GetNumberOfCells(3), level_cells[2]); + BOOST_REQUIRE_EQUAL(mlp.GetNumberOfCells(4), level_cells[3]); +} + +BOOST_AUTO_TEST_CASE(cell_overflow_bits) +{ + // bits = ceil(log2(4194304 + 1)) + ceil(log2(262144 + 1)) + ceil(log2(16384 + 1)) + + // ceil(log2(1024 + 1)) + // = 23 + 19 + 15 + 11 + // = 68 + const size_t NUM_PARTITIONS = 4194304; + const std::vector level_cells = {NUM_PARTITIONS, 262144, 16384, 1024}; + std::vector> levels(level_cells.size(), + std::vector(level_cells[0])); + std::vector levels_to_num_cells(level_cells.size()); + + const auto set_level_cells = [&](size_t level, auto const num_cells) { + for (auto val : util::irange(0ULL, NUM_PARTITIONS)) + { + levels[level][val] = std::min(val, num_cells - 1); + } + levels_to_num_cells[level] = num_cells; + }; + util::for_each_indexed(level_cells.cbegin(), level_cells.cend(), set_level_cells); + + BOOST_REQUIRE_EXCEPTION(MultiLevelPartition(levels, levels_to_num_cells), + util::exception, + [](auto) { return true; }); +} + BOOST_AUTO_TEST_SUITE_END()