Expose the precomputed hash using a trait so that I can use it from rust-selectors.

[emilio]

This allows us to get rid of the extra hashing overhead every time we check the
bloom filter.

---

This change is 

[emilio]

r? @nox

I'm not super-happy about this, but it's the best way I found to keep selectors away from depending on string-cache.

[SimonSapin]

This is not a pre-computed hash. get_hash returns ((self.unsafe_data >> 32) ^ self.unsafe_data) as u32.

[SimonSapin]

unsafe_data is a u64 that contains either:

• A pointer address, for dynamic atoms
• 1_u8 in the lowest byte and 7 bytes of zero-padded inline string, for inline atoms
• 2_u32 in the lower half and an array index (so typically small, with many zero upper bits) in the upper half, for static atoms

In each case, neither unsafe_data itself nor both its halves XORed together is a particularly good hash.

Creating an atom involves hashing the string with SipHasher. For static and dynamic atoms we could keep that hash in respective tables and make get_hash return it. For inline atoms however there is no such table (which is the point).

[emilio]

How would you feel about using the upper bytes for the inline atoms, and storing the hash for the non-inline ones?

[bzbarsky]

@SimonSapin Do you mind answering @emilio's question? This is blocking servo/servo#16070 which is blocking the investigation in https://bugzilla.mozilla.org/show_bug.cgi?id=1348935 so it would be good to get some motion here...

[SimonSapin]

Ah, sorry I let this slip through.

Yes, go ahead. For inline atoms that doesn’t sound like a great hash, but maybe that’s ok. (Is even XORing the two halves too costly?)

[emilio]

Ammended that way, and yeah, for inline atoms xoring should be fine.

r? @SimonSapin

[emilio]

Oh, and if the question re. XORing was wrt the current bloom filter. The answer is that right now we're sip-hashing the hash all the time, which is what this PR is trying to avoid.

[SimonSapin]

Looks good overall. Some details:

---

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/atom.rs, line 180 at r1 (raw file):

    pub disps: &'static [(u32, u32)],
    pub atoms: &'static [&'static str],
    pub hashes: &'static [u64],

Maybe make this [u32], pre-computing the XOR at compile-time and keeping the table smaller?

---

src/atom.rs, line 193 at r1 (raw file):

            disps: &[(0, 0)],
            atoms: &[""],
            hashes: &[0],

Maybe make this &[0x20dd4eb33d9590f2]? (This is what SipHasher::new() returns for me on the empty string.) Or 0x3ddddef3 pre-XORed to 32-bit. Though maybe it doesn’t matter…

---

src/atom.rs, line 271 at r1 (raw file):

    #[inline]
    fn hash<H>(&self, state: &mut H) where H: Hasher {
        state.write_u32(self.get_hash())

I’m not sure about this change. If this is going through another hasher anyway there is not much use re-using the pre-computed hash. And giving 32 bits to that hasher instead of 64 bits seems slightly worse, but maybe that also doesn’t matter?

---

Comments from Reviewable

[emilio]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/atom.rs, line 180 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Maybe make this [u32], pre-computing the XOR at compile-time and keeping the table smaller?

Done.

---

src/atom.rs, line 193 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Maybe make this &[0x20dd4eb33d9590f2]? (This is what SipHasher::new() returns for me on the empty string.) Or 0x3ddddef3 pre-XORed to 32-bit. Though maybe it doesn’t matter…

Sounds good, I did that. I don't think it matters fwiw, but...

---

src/atom.rs, line 271 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

I’m not sure about this change. If this is going through another hasher anyway there is not much use re-using the pre-computed hash. And giving 32 bits to that hasher instead of 64 bits seems slightly worse, but maybe that also doesn’t matter?

Ok, reverted. Also it's unclear to me whether it really matters, but sounds fine.

---

Comments from Reviewable

[SimonSapin]

Thanks!

@bors-servo r+

---

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[emilio]

@bors-servo r=SimonSapin

[emilio]

Oh, great, bors is dead?

[SimonSapin]

@bors-servo r+

Clicked "Synchronize" on http://build.servo.org/homu/queue/string-cache

[bors-servo]

📌 Commit f7ce843 has been approved by SimonSapin

[bors-servo]

⌛ Testing commit f7ce843 with merge a35578c...

[bors-servo]

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing a35578c to master...