Using NonZerou32 as the inner type for Symbol

[cameron1024]

Hi, thanks for the great library 😁 I've been working on a compiler that has many Option<Symbol>s scattered throuhgout. This takes 8 bytes to store, since it needs 4 bytes for the u32, and 4 bytes for the discimminant + padding.

If you replace u32 with NonZeroU32, the compiler can see that the all-0 bit pattern is not valid, so it can use that for the None case, saving 4 bytes:

println!("{}", std::mem::size_of::<u32>());
println!("{}", std::mem::size_of::<Option<u32>>());
println!("{}", std::mem::size_of::<NonZeroU32>());
println!("{}", std::mem::size_of::<Option<NonZeroU32>>());

prints:

4
8
4
4

I've already made this change locally, and it works well, so I'd like to upstream (if it's useful to other people). However, this would be a breaking change, and it feels hard to justify a breaking change for a small (but noticeable) perf hit.

Currently, I see 3 ways of doing this:

• change the function signatures to accept NonZeroU32 instead of u32 (and replace some From impls with TryFrom)
• make the affected APIs panic, and add docs saying that they cannot take 0 as a parameter
• make the APIs simply use u32::MAX when passed 0 (or some other "random" constant)

The first option seems ideal, but is a breaking change to the API signature. The second is what I've implemented in my local change, but is also a breaking change, just without the compiler errors, which is arguably worse. The third option just makes me feel really uneasy.

It's also worth mentioning that I'm not at all familiar with Ruby, and I understand this project is part of a system that needs to be compatible with existing versions of Ruby in some way. If there is a reason why NonZeroU32 is inappropriate here, then it probably makes sense to just fork it. I came across this repo in a comparison of various string interners, and it was the only one that supported PathBuf.

I'd be happy to PR the changes that I've made, but before going further, I thought it would be best to check what the best way forward would be

Thanks 😁

[lopopolo]

Hi Cameron, thanks for opening this issue. I am open to considering this API change but am concerned about the performance impact. The u32 symbols are used to directly index into the underlying Vec in each interner to resolve byte contents. This is an operation that happens quite often in Artichoke Ruby. Replacing the u32 with a NonZeroU32 incurs an XOR or decrement every time a symbol is resolved.

In a future world where a type like NonZero<u32, u32::MAX> is expressible, I'd love to adopt this change and would do a 2.0 for it.

If we go this direction with your proposal, I'd love to borrow the non-max u32 definition from the nonmax crate. It swaps the 0 and MAX cases with an XOR.

Of the proposals you've put forward, my preference is for number 1 with a 2.0.

If the performance impact of not having a native NonMaxU32 type is negligible in artichoke ruby, we can roll forward with this change. If you want to put together a PR, I can put together a benchmark to validate whether the performance is acceptable.

[cameron1024]

Ah that makes sense. Perhaps this is naive/ugly, but is it possible to just insert a dummy value at index 0, then keep using the NonZeroU32 as the index directly?

I'm happy to PR either suggestion, but curious to hear which strategy you think is best?

[lopopolo]

but is it possible to just insert a dummy value at index 0, then keep using the NonZeroU32 as the index directly?

I'd prefer not to do that because then SymbolTable::with_capacity(0) would be forced to allocate.