Skip to content

Fix float bug in reserve_for_insert#348

Open
kraldan wants to merge 1 commit intoboostorg:developfrom
kraldan:fix_reserve_for_insert
Open

Fix float bug in reserve_for_insert#348
kraldan wants to merge 1 commit intoboostorg:developfrom
kraldan:fix_reserve_for_insert

Conversation

@kraldan
Copy link
Copy Markdown

@kraldan kraldan commented May 8, 2026

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
Copy link
Copy Markdown
Member

cmazakas commented May 8, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants