Dedupe vm constants

[rben01]

Builds on #712 by making Constant: Hash so that we can stick it in an IndexSet. Therefore all the deduplication logic can rest solely on the nature of an IndexSet.

This required quite a few changes. Most notably, Number is now opaque instead of providing access to its contained f64 via .0 because it no longer contains an f64, but rather a u64, which is converted to and from an f64 with f6::to_bits/f64::from_bits. But also, we have a new Dummy variant in Constant that stores a distinct value each time it's created. (This is of course necessary for the deduplication logic to work correctly; identical dummies would occupy the same slot in the map.)

[sharkdp]

Thank you for working on this.

Before I review this, I'd like to understand the core motivation better. Is there any upsides to this beside what's mentioned in the PR description at #712 (comment)?

[rben01]

I believe this implementation allows for constant-time lookup of stored values, whereas the referenced implementation uses linear lookup (calls to .iter().position())

[sharkdp]

This required quite a few changes. Most notably, Number is now opaque instead of providing access to its contained f64 via .0 because it no longer contains an f64, but rather a u64, which is converted to and from an f64 with f6::to_bits/f64::from_bits

I'm skeptical about this part. This won't work if we want to replace our number representation, right?

[Goju-Ryu]

I'm skeptical about this part. This won't work if we want to replace our number representation, right?

I was thinking this could be a step in that direction. Disentangling the underlying representation by making it opaque. I might be missing some consequences though.

[sharkdp]

Making it opaque is certainly a good idea, but it sounded like we use the bits of the f64 for the integer? How would that work for some more complex heap-allocated precision datatype?

[rben01]

I'm skeptical about this part. This won't work if we want to replace our number representation, right?

The u64’s only purpose is to be Eq and Hash; any underlying representation that implements those two traits would work just as well. Of course, if we think we'll want to eventually swap in a representation that can't be made Eq or Hash, then we shouldn't implement it here either, and just accept that Number can never go in a Map or Set.

Making it opaque is certainly a good idea, but it sounded like we use the bits of the f64 for the integer? How would that work for some more complex heap-allocated precision datatype?

I believe the only methods that would have to be updated are Number::from_f64 and Number::to_f64, to take into account the new underlying representation. Of course, if we were to use a more capable representation under the hood, then we'd probably want more powerful methods (e.g. a way to get a number more precise than an f64 out), but we wouldn't need them.

Edit: oops, this representation breaks k1 == k2 -> hash(k1) == hash(k2), as -0.0 == +0.0 but they will (probably) hash differently. I assume this is fixable by simply checking if the value is == 0.0 and if it is, return the hash of +0.0.

[sharkdp]

Sorry for the late reply here. I'm still skeptical about the added complexity and potentially decreased flexibility w.r.t. the number format, especially since the benefits don't seem immediately clear (there are no benchmarks that would show better performance, for example). To be honest, I currently also don't have a lot of time to review this, so I'd like to close this for now. If you (or anyone else) thinks that this is a mistake, please feel free to comment here, and we can reconsider.