1484096 - Remove use of `fnv` in bloom.rs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Nicholas Nethercote [inactive]]

To support that, this patch also does the following.

- Removes the insert(), remove() and might_contain() methods, because they are
  specialized versions of insert_hash(), remove_hash(), and
  might_contain_hash(), and they are only used by tests within this file.

- Moves hash() from the top level into create_and_insert_some_stuff().

- Changes create_and_insert_some_stuff() so that instead of hashing consecutive
  integers, it instead hashes stringified consecutive integers, which matches
  real usage a little better.

- Raises the false_positives limit a little to account for the above changes.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove use of `fnv` in bloom.rs

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 9001815 [details] [diff] [review]
Remove use of `fnv` in bloom.rs

Review of attachment 9001815 [details] [diff] [review]:
-----------------------------------------------------------------

Should we also remove the tests of fnv in xpcom/rust/gtest/bench-collections/?

::: servo/components/selectors/bloom.rs
@@ +112,5 @@
>          self.storage.adjust_first_slot(hash, true);
>          self.storage.adjust_second_slot(hash, true);
>      }
>  
> +    /// Removes all items with the particular hash from the bloom filter.

Is "all items" correct here?  This function will decrement the counter for the given hash.

Comment 3

[Nicholas Nethercote [inactive]]

> Should we also remove the tests of fnv in
> xpcom/rust/gtest/bench-collections/?

I'm inclined to leave it, because it's a widely-used hashing function, and it's good to have measurements of fxhash against it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15fd461d2ec
Remove use of `fnv` in bloom.rs. r=heycam

Comment 5

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/f15fd461d2ec