Skip to content

Bug: IndexMap fails to insert more than u16::MAX keys. #579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ValouBambou opened this issue May 24, 2025 · 3 comments
Open

Bug: IndexMap fails to insert more than u16::MAX keys. #579

ValouBambou opened this issue May 24, 2025 · 3 comments

Comments

@ValouBambou
Copy link

ValouBambou commented May 24, 2025

I ran into this bug when inserting a lot of keys in an IndexMap, and it turns out that when my keys are bigger than an u16 they are not inserted properly. Here is a minimal reproducible code.

use heapless::FnvIndexMap;

fn main() {
    const N: usize = 1 << 17;
    let mut map = FnvIndexMap::<u32, usize, N>::new();
    for i in 1..(0xffff + 3) {
        let block = i;
        let idx = i as usize - 1;
        map.insert(block, idx).unwrap();
        assert!(map.contains_key(&block), "{}", i);
    }
}

I have no idea on why this happens. Does anyone have an idea ?
EDIT: the issue is not the values but the number of keys to insert shifting the start and end of the loop leads to the same bug
EDIT 2: the bug is present in the 0.8.0 version but fixed in 0.9.0 and on the latest commit on master.
EDIT 3: the bug is not fixed in any versions

@ValouBambou ValouBambou changed the title Bug: IndexMap fails to insert properly value bigger than u16::MAX Bug: IndexMap fails to insert more than u16::MAX keys. May 24, 2025
@ValouBambou
Copy link
Author

ValouBambou commented May 24, 2025

I see that the new release fixing this bug is not published to creates.io is this intentional ?
EDIT: I saw #568 and understand why the 0.9.0 release is not ready to ship. So I will close this. But maybe doing a 0.8.1 release with fixes including this one could be nice.

@ValouBambou
Copy link
Author

I noticed that the code path with exit if dist is too long in find

return None;
is taken but removing the hole if statement doesn't solve the issue, so I guess there is more to it. I've not figured out how the algorithm works in details, maybe replacing the hash value with u32 is enough, but I've not tried. Even if it worked, we should probably rethink it with a LenT type to avoid increasing the size from u16 to u32 when it's not necessary.

@ValouBambou
Copy link
Author

I tried in a fork to replace u16 by u32 for the hash and pos, and it seems to work. So I guess it is possible to make a smarter system based on the size of the const N:usize like it is done in Vec.

Also, even if this is too much work there is a quick fix solution, that doesn't solve the problem but at least warn people about the issue would be to add an assert!(CAPACITY < (1 << 16)) in this line

assert!(N.is_power_of_two());

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

No branches or pull requests

1 participant