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.

Author: mjjbell

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

include/partitioner/multi_level_partition.hpp

@@ -186,11 +186,12 @@ template <storage::Ownership Ownership> class MultiLevelPartitionImpl final
             auto bits = static_cast<std::uint64_t>(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 <storage::Ownership Ownership> 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;
                             });

unit_tests/partitioner/multi_level_partition.cpp

@@ -1,6 +1,8 @@
 #include <boost/numeric/conversion/cast.hpp>
 #include <boost/test/unit_test.hpp>
 
+#include "util/exception.hpp"
+#include "util/for_each_indexed.hpp"
 #include <util/integer_range.hpp>
 #include <util/msb.hpp>
 
@@ -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<size_t> level_cells = {NUM_PARTITIONS, 258451, 16310, 534};
+    std::vector<std::vector<CellID>> levels(level_cells.size(),
+                                            std::vector<CellID>(level_cells[0]));
+    std::vector<uint32_t> levels_to_num_cells(level_cells.size());
+
+    const auto set_level_cells = [&](size_t level, auto const num_cells) {
+        for (auto val : util::irange<size_t>(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<size_t> level_cells = {NUM_PARTITIONS, 262144, 16384, 1024};
+    std::vector<std::vector<CellID>> levels(level_cells.size(),
+                                            std::vector<CellID>(level_cells[0]));
+    std::vector<uint32_t> levels_to_num_cells(level_cells.size());
+
+    const auto set_level_cells = [&](size_t level, auto const num_cells) {
+        for (auto val : util::irange<size_t>(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()