1176160 - Remove PL_DHashTableEnumerate() use from rdf/

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Nicholas Nethercote [inactive]]

Because PLDHashTable::Iterator is much nicer than PL_DHashTableEnumerate().

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove simple uses of PL_DHashTableEnumerator() from rdf/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove uses of PL_DHashTableEnumerator() involving VisitorClosure from rdf/

I've preserved the existing behaviour, which is that if NS_RDF_STOP_VISIT
occurs in the inner loop, we break out of the inner loop and continue the outer
loop. But it's possible that the intended behaviour is to break out of both
loops, as happens if NS_FAILED() is true. (This latter behaviour would actually
be easier to implement because the |inner_end| label could be removed.)

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove uses of PL_DHashTableEnumerator() from SweepForwardArcsEntries()

Comment 4

[Axel Hecht]

Comment on attachment 8624612 [details] [diff] [review]
(part 1) - Remove simple uses of PL_DHashTableEnumerator() from rdf/

Sorry for the lag, mountains in my way. Patches like this are really better for :bs to review, redirecting.

Comment 5

[Benjamin Smedberg]

Comment on attachment 8624613 [details] [diff] [review]
(part 2) - Remove uses of PL_DHashTableEnumerator() involving VisitorClosure from rdf/

oh god this is so awful. Arbitrary visitor code on the stack while enumerating a hashtable :-(

But you're not making it worse, and I'm hoping to just rm -rf rdf/ one of these days anyway.

Comment 6

[Benjamin Smedberg]

Comment on attachment 8624614 [details] [diff] [review]
(part 3) - Remove uses of PL_DHashTableEnumerator() from SweepForwardArcsEntries()

Is this possibly safe? SweepForwardArcsEntries seems to call itself recursively while iterating over a hashtable, which ought to invalidate the iterator, no?

Comment 7

[Nicholas Nethercote [inactive]]

Thank you for the fast reviews.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 8624614 [details] [diff] [review]
> (part 3) - Remove uses of PL_DHashTableEnumerator() from
> SweepForwardArcsEntries()
> 
> Is this possibly safe? SweepForwardArcsEntries seems to call itself
> recursively while iterating over a hashtable, which ought to invalidate the
> iterator, no?

First of all, I've tried very hard to simply preserve the existing behaviour. I've done a lot of enuerate-to-iterator conversions now so I'm confident I've done so here as well :)

Addressing your concern more directly... AIUI, there is a hash table, each element of which can contain a hash table, but it can only go one level deep, as shown by this comment:

> Stuff in sub-hashes must be swept recursively (max depth: 1)

(Indeed, my "part 2" patch modifies other code that handles this data structure via a nested loop rather than via recursion, so the "must be" in this comment isn't strictly true.)

More importantly, a new iterator is used for each sub-table, so there should be no invalidation issues.

Basically it's a just a N-ary tree with one or two levels and a new iterator is used for each node.

Comment 8

[Nicholas Nethercote [inactive]]

Also, I've done a try push that looked fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=811e0f45d012.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9cb580a041
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f83d890004e

Comment 10

[Nicholas Nethercote [inactive]]

Still one patch to go.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea9cb580a041
https://hg.mozilla.org/mozilla-central/rev/1f83d890004e

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a11da784c65

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a11da784c65