Proposal: Replace wrapping_add with checked_add

[icefoxen]

u16 seems uncomfortably small a generation size for me to be sure that I won't overflow it by accident if I have, I dunno, a particle system that adds and removes lots of objects rapidly. It looks like you use wrapping_add for all the generation increments, so it appears that a generation that gets maxed out will just wrap around, possibly making stale handles look alive again. This PR (hopefully) replaces them all with checked_add() followed by an expect(). This makes it a dynamically-checked error for a generation number to overflow.

Whether you want this is up to you, I just wanted to offer the suggestion.

[thomcc]

Hm, the code intends to handle generation overflow relatively gracefully. The downside of the way we handle generation overflow is just that we might fail to detect a conflict in the case where you have a handle from (an exact multiple of) 32767 generations previous. I think that's better behavior since that case seems very rare, and while it could result in a slight logic error, no memory unsafety would result.

[icefoxen]

I mean... From my perspective it doesn't handle generation overflow at all, it just fixes it up and keeps on trucking. So using it a certain way makes it possible to create silent logic errors, and there's no way for the user to check for the error themselves.

It's your code, you can do what you want. It just seems an odd case to deliberately work around. It introduces a case where, if you actually need the generation counter, there's a tiny chance of it just not working correctly.

[thomcc]

The handling is mostly in ensuring the generation value isn't 0. The map is permanently broken on generation overflow with your suggestion, wheras the vast majority of use cases that hit generation overflow would not experience a problem here -- it's only a problem if you both

1. hit generation overflow.
2. happen to have a handle which is exactly 32767 generations behind that refers to the same index. (okay, if you had generation overflow twice, and had a handle from 65534 generations behind, it would also have the same problem, etc)

I agree it's not ideal, but as you mention, 2^16 is low enough that I think generation overflow is entirely possible, and would like to avoid crashes in all other cases, just because there might be a problem in that specific case (which feels extremely rare to me).

[icefoxen]

It's just a matter of what you want. In my experience, if I hit generation overflow once then I'll probably hit it multiple times, and I would far rather have something panic and stop the program than give me invalid results 0.003% of the time. That sounds like a bug that would be extremely difficult to find.

[thomcc]

If hitting generation overflow guaranteed invalid results, I'd agree, but you also have to have a handle from when that slot was exactly the previous generation, which seems very rare.

I'm open to suggestions for how to deal with this that don't cause panics for all generation overflows (since the vast majority of them should be fine), however, but I can't think of a way to do that.

[thomcc]

I'd also be open to this being a flag on the HandleMap you can enable if you desire.

[icefoxen]

The course I would take is to make generation a u32 and document that this panics if it overflows. But that would probably not be what you want since you'd have to either make handles bigger or remove the 16-bit "extra" value from it (which would be my decision; handles can be distinguished by wrapping them in newtypes).

Beyond that, it's just a matter of principle. If a data structure is designed for robustness, I don't want it to have a small chance of silently giving you the wrong data, without documenting this behavior. I would rather have it just fail noisily.

[thomcc]

handles can be distinguished by wrapping them in newtypes

Strongly disagree, and have faced several bugs where this would not be possible. For example, in https://github.com/mozilla/application-services often the handle maps were the same instance of a global handle map that happened to be loaded in separate .so files. That sort of detection must be performed via dynamic checking.

This library also provides TypedHandleMap if you want to use static checking, and I guess I could be convinced that for TypedHandleMap it should use a different handle type with a 32 bit generation count and no map ID.

[icefoxen]

Welp, up to you.

[thomcc]

If you want maps with the behavior you're describing, the slotmap crate already exists, and:

• has 32 bit generations (which is large enough that generation overflow probably doesn't matter
• no map ids, but better support for newtyped keys.
• now that DenseSlotMap is back doesn't require T: Copy for one of it's handle types

It sounds like that's the crate you really want to use. I'll consider increasing the generation field size, but honestly I don't think catastrophic failure in the case of generation overflow would be acceptable with a u16 generation.

If I were going to do that I'd also want to tweak the index generation algorithm to prefer low generation numbers instead of it's current algorithm (which prefers densely packed indices), which seems like it might be difficult to do as efficently.

[thomcc]

I will make an effort to better document this shortcoming though, but it will be probably a week or so before I have time to update it.

I'll also consider a flag that handles overflow the way you desire, (this should be doable without a major performance hit, as we already have the overflow check in the gen == 0 case)