1481138 - Smooth some rough edges off the mozilla::HashTable API

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1481138 - Remove the add() variant in HashTable and GCHashTable that uses a default value.

Because (a) it's kinda weird, and (b) only used in a single test, where it can
be easily replaced with a vanilla add().

Review commit: https://reviewboard.mozilla.org/r/261516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261516/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1481138 - Remove HashMap::lookupWithDefault().

Because it's quite strange, badly named, not that useful, and barely used.

Also remove WeakMap::lookupWithDefault(), which is similar, but not used at
all.

Review commit: https://reviewboard.mozilla.org/r/261518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261518/

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1481138 - Remove Hash{Map,Set}::putNewInfallible.

The difference between putNew() and putNewInfallible() is subtle -- the latter
omits three checks. Omitting two of them (SimulatedOOM and checkOverloaded())
is reasonable, but omitting the EnsureHash() check is dubious.

Furthermore, putNewInfallible() has very few uses; in my opinion, not enough to
warrant its existence. So this patch removes it, simplifying the API and the
implementation.

Review commit: https://reviewboard.mozilla.org/r/261520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261520/

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1481138 - Clarify that Hash{Map,Set}::putNew() can be used if elements have been removed.

Hash{Map,Set}::putNew() can be used on a table that has had elements removed,
despite some comments to the contrary.

This patch fixes those comments.

This patch also removes the !isRemoved() assertion in findFreeEntry(), which is
confusing -- !isLive() would be more precise, but also obvious from the
surrounding code.

Review commit: https://reviewboard.mozilla.org/r/261522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261522/

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8997782 [details]
Bug 1481138 - Remove the add() variant in HashTable and GCHashTable that uses a default value.

https://reviewboard.mozilla.org/r/261516/#review268568

Hah, agreed.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8997783 [details]
Bug 1481138 - Remove HashMap::lookupWithDefault().

https://reviewboard.mozilla.org/r/261518/#review268570

Yeah, much more readable as two operations.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8997784 [details]
Bug 1481138 - Remove Hash{Map,Set}::putNewInfallible.

https://reviewboard.mozilla.org/r/261520/#review268576

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8997785 [details]
Bug 1481138 - Clarify that Hash{Map,Set}::putNew() can be used if elements have been removed.

https://reviewboard.mozilla.org/r/261522/#review268578

Nice!

Comment 9

[Nicholas Nethercote [inactive]]

Ugh, the third patch causes test failures. It's because putNew() contains a checkSimulatedOOM() call while putNewInfallible() does not. So the jit-tests harness inserts some OOMs and those MOZ_ALWAYS_TRUE() assertions I inserted fail. Bleh.

I think I will abandon patch 3 and keep putNewInfallible(), but comment it better in patch 4.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d5dcbe36ff
Remove the add() variant in HashTable and GCHashTable that uses a default value. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/01dcd8f80341
Remove HashMap::lookupWithDefault(). r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/09cd991472c9
Clarify that Hash{Map,Set}::putNew() can be used if elements have been removed. r=luke

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/49d5dcbe36ff
https://hg.mozilla.org/mozilla-central/rev/01dcd8f80341
https://hg.mozilla.org/mozilla-central/rev/09cd991472c9