Fix float bug in reserve_for_insert

[kraldan]

Intro

Hi, I found practically the same bug as in boostorg/multi_index#94.

reserve_for_insert uses floats to calculate the next number of buckets. These can round down and not increase the bucket count, which leads the table being rehashed on multiple subsequent inserts that rely on this method.

The following program demonstrates this.

Code

#include <boost/unordered/unordered_set.hpp>

#include <chrono>
#include <cstddef>
#include <format>
#include <iostream>

using time_unit = std::chrono::milliseconds;

void note_time(time_unit time, std::size_t container_size)
{
    static unsigned long max_ms = 0;
    const double tol = 0.25;

    const unsigned long ms =
        std::chrono::duration_cast<std::chrono::milliseconds>(time).count();

    if (ms > max_ms)
        max_ms = ms;

    if (ms >= 1 && ms >= max_ms * tol)
        std::cout << std::format("container size: {}, insert time: {} ms\n",
                                 container_size, ms);
}

int main()
{
    static constexpr int range1_lo = 201326605, range1_hi = 201326630;
    static constexpr int range2_lo = 402653179, range2_hi = 402653220;

    boost::unordered_set<int> a;
    for (int i = range1_lo; i <= range1_hi; ++i) a.insert(i);
    for (int i = range2_lo; i <= range2_hi; ++i) a.insert(i);

    boost::unordered_set<int> b;
    for (int i = 0; i < 405'000'000; ++i) {
        using clock = std::chrono::steady_clock;
        auto t0 = clock::now();

        if ((i >= range1_lo && i <= range1_hi) ||
            (i >= range2_lo && i <= range2_hi)) {
            b.insert(a.extract(i));
        } else {
            b.insert(i);
        }

        auto t1 = clock::now();
        note_time(std::chrono::duration_cast<time_unit>(t1 - t0), b.size());
    }
}

(I'm sorry for it being memory-heavy, it was the simplest way for me to demonstrate.)

On my machine, with the current implementation, the output is:

container size: 98318, insert time: 1 ms
container size: 196614, insert time: 2 ms
container size: 393242, insert time: 5 ms
container size: 786434, insert time: 11 ms
container size: 1572870, insert time: 22 ms
container size: 3145740, insert time: 46 ms
container size: 6291470, insert time: 101 ms
container size: 12582918, insert time: 180 ms
container size: 25165844, insert time: 355 ms
container size: 50331654, insert time: 729 ms
container size: 100663320, insert time: 1686 ms
container size: 201326612, insert time: 2113 ms
container size: 201326613, insert time: 2138 ms
container size: 201326614, insert time: 2259 ms
container size: 201326615, insert time: 2121 ms
container size: 201326616, insert time: 3139 ms
container size: 402653190, insert time: 5819 ms
container size: 402653191, insert time: 4343 ms
container size: 402653192, insert time: 4516 ms
container size: 402653193, insert time: 4377 ms
container size: 402653194, insert time: 4263 ms
container size: 402653195, insert time: 4329 ms
container size: 402653196, insert time: 4225 ms
container size: 402653197, insert time: 4229 ms
container size: 402653198, insert time: 4220 ms
container size: 402653199, insert time: 4322 ms
container size: 402653200, insert time: 4181 ms
container size: 402653201, insert time: 9137 ms

On my machine, with the the fix, the output is:

container size: 98318, insert time: 1 ms
container size: 196614, insert time: 2 ms
container size: 393242, insert time: 5 ms
container size: 786434, insert time: 11 ms
container size: 1572870, insert time: 23 ms
container size: 3145740, insert time: 48 ms
container size: 6291470, insert time: 93 ms
container size: 12582918, insert time: 183 ms
container size: 25165844, insert time: 363 ms
container size: 50331654, insert time: 719 ms
container size: 100663320, insert time: 1476 ms
container size: 201326612, insert time: 2947 ms
container size: 402653190, insert time: 7719 ms

The fix

I implemented the fix to mirror https://github.com/boostorg/multi_index/pull/94/changes

[cmazakas]

Can you please update your branch to add a test that fails without this change in one commit and then fix in the following commit?

You can run b2 easily from the boost root just using ./b2 libs/unordered/test//test-file, iirc. I'm a little rusty with b2 nowadays but that should work.

[joaquintides]

Can you please update your branch to add a test that fails without this change in one commit and then fix in the following commit?

The problem with regression testing this in a black-box fashion is that you need more than 5GB of RAM to hit the first problematic case, not sure this would go well with our CI. I'll see if I can do produce some intrusive test for this that does not actually allocate memory, failing that I'm inclined to just accept the PR and merge.

[cmazakas]

Can you please update your branch to add a test that fails without this change in one commit and then fix in the following commit?

The problem with regression testing this in a black-box fashion is that you need more than 5GB of RAM to hit the first problematic case, not sure this would go well with our CI. I'll see if I can do produce some intrusive test for this that does not actually allocate memory, failing that I'm inclined to just accept the PR and merge.

Hmm, can we passively test it using a custom Allocator that tracks sizes?