1181445 - Implement iterators for nsTHashtable and nsBaseHashtable and use them in xpcom/

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

PLDHashTable got an iterator; its subclasses nsTHashtable and nsBaseHashtable deserve one too.

As part of this bug I'll also convert the Enumerate()-style calls in xpcom/ to use the new iterators. This will give the new iterators a good workout to make sure they're designed reasonably.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Implement iterators for nsTHashtable and nsBaseHashtable

nsBaseHashtable has both EnumerateRead() and Enumerate(). A comment claims that
the latter locks the table, but this is false, so I removed the comment. Other
than that the only notable difference between them is that they have slightly
different types for dealing with values (|UserDataType| vs |DataType&|) so I've
implemented both GetUserData() and GetData(), allowing either type to be used.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use nsTHashTable::Iterator in nsMemoryReporterManager

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use nsTHashTable::Iterator in nsObserverService

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use nsTHashTable::Iterator in nsCategoryManager

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use nsTHashTable::Iterator in nsTHashtable.h

This is a particularly nice example of how iterators can be so much nicer than
Enumerate()-style functions:

  1 file changed, 4 insertions(+), 33 deletions(-)

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Use nsBaseHashTable::Iterator in xpcom/components/

Note that enumfunc_pentries and persistent_userstruct are unused, and so could
be removed.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Use nsBaseHashTable::Iterator in xpcom/reflect/

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Use nsBaseHashTable::Iterator in SystemMemoryReporter

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Use nsBaseHashTable::Iterator in nsConsoleService

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Use nsBaseHashTable::Iterator in CycleCollectedJSRuntime

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Use nsBaseHashTable::Iterator in xpcom/glue/

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Use nsBaseHashTable::Iterator in xpcom/threads/

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Use nsBaseHashTable::Iterator in xpcom/ds/

Comment 14

[Nicholas Nethercote [inactive]]

froydnj: apologies for the review dump. There's no rush on this.

For parts 2--13, the diffstats are as follows:

> 20 files changed, 253 insertions(+), 521 deletions(-)

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use nsTHashTable::Iterator in nsObserverService

I tweaked some variable names.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8631418 [details] [diff] [review]
(part 1) - Implement iterators for nsTHashtable and nsBaseHashtable

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

I wonder if future patches could make the UserData/Data distinction go away...

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8631419 [details] [diff] [review]
(part 2) - Use nsTHashTable::Iterator in nsMemoryReporterManager

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1434,5 @@
>    {
>      mozilla::MutexAutoLock autoLock(mMutex);
> +    for (auto iter = mStrongReporters->Iter(); !iter.Done(); iter.Next()) {
> +      nsRefPtrHashKey<nsIMemoryReporter>* entry = iter.Get();
> +      allReporters.AppendElement(entry->GetKey());

I have sometimes wondered whether we shouldn't have:

nsTArray<T> Items();

or similar in our hash table classes...

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8631451 [details] [diff] [review]
(part 3) - Use nsTHashTable::Iterator in nsObserverService

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

::: xpcom/ds/nsObserverService.cpp
@@ +75,5 @@
>  NS_IMETHODIMP
>  nsObserverService::CollectReports(nsIHandleReportCallback* aHandleReport,
>                                    nsISupports* aData, bool aAnonymize)
>  {
>    ObserverServiceReferentCount referentCount;

Maybe file a followup for turning this into a collection of local variables?  I think that would consolidate the num{Strong,WeakAlive,WeakDead} handling below...

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 8631428 [details] [diff] [review]
(part 5) - Use nsTHashTable::Iterator in nsTHashtable.h

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

Nice.

Comment 20

[Andrew McCreight [:mccr8]]

Comment on attachment 8631443 [details] [diff] [review]
(part 10) - Use nsBaseHashTable::Iterator in CycleCollectedJSRuntime

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

Hooray.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +477,5 @@
>  CycleCollectedJSRuntime::UnmarkSkippableJSHolders()
>  {
> +  for (auto iter = mJSHolders.Iter(); !iter.Done(); iter.Next()) {
> +    void* holder = iter.GetKey();
> +    nsScriptObjectTracer*& tracer = iter.GetData();

No real need for these intermediate variables, but this is fine, too.

@@ +657,5 @@
>    // NB: This is here just to preserve the existing XPConnect order. I doubt it
>    // would hurt to do this after the JS holders.
>    TraverseAdditionalNativeRoots(aCb);
>  
>    Closure closure(&aCb);

This closure can be disentangled more but I can file a followup for that if you don't want to bother. In short: closure.mCb is aCb and the closure for TraceCallbackFunc can just become bool*. I just mashed together two different closures for convenience.

@@ +817,5 @@
>  
> +  for (auto iter = mJSHolders.Iter(); !iter.Done(); iter.Next()) {
> +    void* holder = iter.GetKey();
> +    nsScriptObjectTracer*& tracer = iter.GetData();
> +    tracer->Trace(holder, JsGcTracer(), aTracer);

As above, the intermediate variables don't add much in my mind, but this is fine, too.

Comment 21

[Nicholas Nethercote [inactive]]

Thank you for the fast reviews.

> I wonder if future patches could make the UserData/Data distinction go
> away...

I don't know. The UserData/Data distinction is baked deeply into nsBaseHashtable:

> template<class KeyClass, class DataType, class UserDataType>
> class nsBaseHashtable;

And GetData() returns a reference, while GetUserData() does not. My experiences with PLDHashTable refactoring has led me to be conservative with these kinds of changes, which is why I maintained the split.

> Maybe file a followup for turning this into a collection of local variables?
> I think that would consolidate the num{Strong,WeakAlive,WeakDead} handling
> below...

Good idea. I'll do it here, because this kind of unnecessary struct removal is very much in line with other changes made in this bug.

Comment 22

[Nicholas Nethercote [inactive]]

> > +  for (auto iter = mJSHolders.Iter(); !iter.Done(); iter.Next()) {
> > +    void* holder = iter.GetKey();
> > +    nsScriptObjectTracer*& tracer = iter.GetData();
> 
> No real need for these intermediate variables, but this is fine, too.

I started off not using intermediates all that much. But then the more of these changes I made, the more I thought having the intermediates was a good idea -- in order to make the key/data types obvious, as is the case in the enumerate functions -- and I actually went back and added some intermediates in where I didn't have them previously. And this is doubly true for the GetData() cases where it's actually a reference being returned, which is not obvious.

> >    Closure closure(&aCb);
> 
> This closure can be disentangled more but I can file a followup for that if
> you don't want to bother.

I filed bug 1182318.

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use nsTHashTable::Iterator in nsObserverService

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8631886 [details] [diff] [review]
(part 3) - Use nsTHashTable::Iterator in nsObserverService

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

Here's the version of part 3 that removes ObserverServiceReferentCount. Carrying over r+ because it's not a big change (though it is a nice one).

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/272eac79edf0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d73570c5e142
https://hg.mozilla.org/integration/mozilla-inbound/rev/4469946f2f7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf12b557311
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d67a70850f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f49f709e0b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc7732841f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b92757e5e21
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d871a96abaa
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c437b6b369
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ac7e9fcea4
https://hg.mozilla.org/integration/mozilla-inbound/rev/64040ac92a51
https://hg.mozilla.org/integration/mozilla-inbound/rev/961059b7073e

Comment 26

[Nicholas Nethercote [inactive]]

And try looked good prior to landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c620981ea645

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/272eac79edf0
https://hg.mozilla.org/mozilla-central/rev/d73570c5e142
https://hg.mozilla.org/mozilla-central/rev/4469946f2f7b
https://hg.mozilla.org/mozilla-central/rev/2bf12b557311
https://hg.mozilla.org/mozilla-central/rev/9d67a70850f2
https://hg.mozilla.org/mozilla-central/rev/7f49f709e0b8
https://hg.mozilla.org/mozilla-central/rev/2dc7732841f4
https://hg.mozilla.org/mozilla-central/rev/3b92757e5e21
https://hg.mozilla.org/mozilla-central/rev/7d871a96abaa
https://hg.mozilla.org/mozilla-central/rev/78c437b6b369
https://hg.mozilla.org/mozilla-central/rev/39ac7e9fcea4
https://hg.mozilla.org/mozilla-central/rev/64040ac92a51
https://hg.mozilla.org/mozilla-central/rev/961059b7073e

Comment 28

[Markus Stange [:mstange]]

Was it an intentional decision to make the iterators not support ranged-based for loops? It would be nice if we could replace
for (auto iter = mTagged.Iter(); !iter.Done(); iter.Next()) {
with
for (auto iter : mTagged) {

Comment 29

[Nicholas Nethercote [inactive]]

> Here's the version of part 3 that removes ObserverServiceReferentCount.
> Carrying over r+ because it's not a big change (though it is a nice one).

... and I messed that up and caused bug 1182926, alas.

Comment 30

[Nicholas Nethercote [inactive]]

(In reply to Markus Stange [:mstange] from comment #28)
> Was it an intentional decision to make the iterators not support
> ranged-based for loops? It would be nice if we could replace
> for (auto iter = mTagged.Iter(); !iter.Done(); iter.Next()) {
> with
> for (auto iter : mTagged) {

Bug 1180122 is going to reinstate Chaos Mode support for PLDHashTable::Iterator. This means that the starting point for hash table iteration is potentially randomized, and with that in place I don't see how the required begin(), end() and operator!=() methods can be made to work as required for range-based for loops.

This doesn't worry me that much. Going from enumerate-style functions to iteration is a big improvement. Going from traditional iterator loops to range-based loops is a much smaller improvement.

Comment 31

[Markus Stange [:mstange]]

(In reply to Nicholas Nethercote [:njn] from comment #30)
> (In reply to Markus Stange [:mstange] from comment #28)
> > Was it an intentional decision to make the iterators not support
> > ranged-based for loops? It would be nice if we could replace
> > for (auto iter = mTagged.Iter(); !iter.Done(); iter.Next()) {
> > with
> > for (auto iter : mTagged) {
> 
> Bug 1180122 is going to reinstate Chaos Mode support for
> PLDHashTable::Iterator. This means that the starting point for hash table
> iteration is potentially randomized, and with that in place I don't see how
> the required begin(), end() and operator!=() methods can be made to work as
> required for range-based for loops.
> 
> This doesn't worry me that much. Going from enumerate-style functions to
> iteration is a big improvement. Going from traditional iterator loops to
> range-based loops is a much smaller improvement.

That makes sense. I agree that having iterators at all is a much bigger improvement. Thanks!

Comment 32

[Nicholas Nethercote [inactive]]

FWIW, bug 1149833 is open for doing range-based iterators for the hash tables. It's probably possible, it'll just require some thinking.