1348600 - stylo: Need to convert UndisplayedMap to a more-modern hashtable without side-effect-y lookup

Status: RESOLVED DUPLICATE of bug 1296516

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

Bug 1294915 comment 51 reveals that PL_HashTable does a cute (read: terrifying) optimization whereby it reorders the entries during lookup:

http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/nsprpub/lib/ds/plhash.c#149

This is bad for stylo, because this ancient hashtable is used by UndisplayedMap, which we need to read during the parallel traversal. I talked to Nathan, and he suggested that we should probably just switch this to a more-modern hashtable, presumably an nsClassHashTable with a UniquePtr<UndisplayedNode> as the value type.

This should be pretty straightforward, but is slightly too large of a task for me to do synchronously while triaging this stuff.

Comment 1

[Bobby Holley (:bholley)]

Nick, you have a lot of experience upgrading hashtables. Is this something someone you or someone on your team could knock out in the next few weeks?

Comment 2

[Cameron McCormack (:heycam)]

Emilio has a patch in bug 1296516 to make mUndisplayedMap use nsTHashtable, which I deferred reviewing at in lieu of higher priority things.  Is that all we need?

Comment 3

[Bobby Holley (:bholley)]

Yes!

Comment 4

[Nicholas Nethercote [inactive]]

I have zero experience with PL_HashTable. Converting this particular table to PLDHashTable or nsTHashtable sounds wise.

Comment 5

[Eric Rahm [:erahm]]

I'm not 100% sure what issue you're trying to avoid: accessing mutates the table or expecting a stable iteration order. Just note that we make no guarantees of ordering in PLDHashTable, in fact we intentionally make things random in chaos mode [1].

[1] http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/xpcom/ds/PLDHashTable.cpp#728-734

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #5)
> I'm not 100% sure what issue you're trying to avoid: accessing mutates the
> table or expecting a stable iteration order.

This bug is trying to avoid the former, since reordering the table on lookup when you're touching the table from multiple threads is a recipe for badness.

Comment 7

[Bobby Holley (:bholley)]

This is getting taken care of in bug 1296516. Thanks Emilio, you rock. :-)