1166598 - Remove more PL_DHashTable{Init,Finish}() calls

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

Like bug 1166586. These ones are marginally trickier, mostly involving a call to PL_DHashTableInit() in an Init() function. But still pretty straightforward.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use PLDHashTable2 in nsScriptNameSpaceManager

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use PLDHashTable2 in SpanningCellSorter

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use PLDHashTable2 in nsCommandParams

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use PLDHashTable2 in RDFServiceImpl

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use PLDHashTable2 in InMemoryDataSource

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Clean up nsStaticCaseInsensitiveNameTable

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Use PLDHashTable2 in nsLoadGroup

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Use PLDHashTable2 in nsHostResolver

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Use PLDHashTable2 in nsTHashtable

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

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

r=me with either splitting out the bit below to a different bug or getting somebody to comment on the goodness of it.

::: layout/tables/SpanningCellSorter.cpp
@@ -150,5 @@
>                  HashTableEntry **sh =
>                      new HashTableEntry*[mHashTable.EntryCount()];
> -                if (!sh) {
> -                    // give up
> -                    mState = DONE;

Clearly this can be removed in its current state, but I think it'd be worth it to get a layout person to confirm that we want to infallibly allocate here, since the hunks that I can see seem to make some effort towards handling allocation failures gracefully.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8608437 [details] [diff] [review]
(part 5) - Use PLDHashTable2 in InMemoryDataSource

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

::: rdf/base/nsInMemoryDataSource.cpp
@@ +754,5 @@
>      NS_ADDREF(datasource);
>  
> +    datasource->fAggregated.AddRef();
> +    nsresult rv = datasource->AggregatedQueryInterface(aIID, aResult); // This'll AddRef()
> +    datasource->fAggregated.Release();

This code...

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8608440 [details] [diff] [review]
(part 7) - Use PLDHashTable2 in nsLoadGroup

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

::: netwerk/base/nsLoadGroup.h
@@ +49,5 @@
>      ////////////////////////////////////////////////////////////////////////////
>      // nsLoadGroup methods:
>  
>      explicit nsLoadGroup(nsISupports* outer);
> +    virtual ~nsLoadGroup();

Um.  I guess this doesn't hit the public-destructor checks in nsISupportsImpl.h because we use a different set of macros to define AddRef/Release for this class?

Can you file followup bugs for:

a) adding the same MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING to the macros in nsAgg.h;
b) making NS_GENERIC_AGGREGATED_CONSTRUCTOR not require access to the destructor?  (I think we can do this by use the same idea as NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT.).

These aren't terribly important bugs, as there's not that many aggregated classes in m-c (part (a) above might break comm-central, though), but a little more safety never hurt anybody.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8608439 [details] [diff] [review]
(part 6) - Clean up nsStaticCaseInsensitiveNameTable

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

I think this could have been several patches, but whatever.

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

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

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #10)
> Comment on attachment 8608433 [details] [diff] [review]
> (part 2) - Use PLDHashTable2 in SpanningCellSorter
> 
> Review of attachment 8608433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with either splitting out the bit below to a different bug or getting
> somebody to comment on the goodness of it.
> 
> ::: layout/tables/SpanningCellSorter.cpp
> @@ -150,5 @@
> >                  HashTableEntry **sh =
> >                      new HashTableEntry*[mHashTable.EntryCount()];
> > -                if (!sh) {
> > -                    // give up
> > -                    mState = DONE;
> 
> Clearly this can be removed in its current state, but I think it'd be worth
> it to get a layout person to confirm that we want to infallibly allocate
> here, since the hunks that I can see seem to make some effort towards
> handling allocation failures gracefully.

dholbert, would you mind taking a look? Thanks.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

Yeah, it looks like we might really want fallible allocation there.

I'm unfamiliar with SpanningCellSorter; I'll punt to dbaron, who wrote this code (ages ago) according to https://hg.mozilla.org/users/gszorc_mozilla.com/gecko-full/annotate/c9de915dc050/layout/tables/SpanningCellSorter.cpp#l152

(dbaron, see comment 10.)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think the OOM-handling done by SpanningCellSorter (and the lack of result-checking by AddCell's caller) amounts to any useful OOM-handling, so I think it should probably be infallible.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

r=dbaron, although my one comment would be that in most cases in the current code this hash table will *not* get initialized.  It will only get initialized if the table has a cell with colspan >= 10.  So my only concern here would not be the infallible allocation (which is fine, see previous comment), but any increased costs of initialization and destruction.  Maybe they're too small to worry about, though.

(The whole point of SpanningCellSorter is to enumerate all cells with colspans in a table, sorted by their colspan.)

Comment 18

[Nicholas Nethercote [inactive]]

> r=dbaron, although my one comment would be that in most cases in the current
> code this hash table will *not* get initialized.  It will only get
> initialized if the table has a cell with colspan >= 10.  So my only concern
> here would ... be ... any increased costs of initialization and destruction.  Maybe
> they're too small to worry about, though.

I just did some simple instrumentation and in a couple of minutes of browsing across a few sites only a few thousand SpanningCellSorter classes were created. Seems few enough to not cause a problem. If it does, allocating the hash table on the heap will be an option.

Comment 19

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Created attachment 8608442 [details] [diff] [review]
> (part 9) - Use PLDHashTable2 in nsTHashtable

Well, this is odd. This patch works fine on all platforms but Windows, where it lights up Treeherder like a Skittles commercial: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4186e372a1c8

I may land the first eight patches and leave this one for later.

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> >
> > Created attachment 8608442 [details] [diff] [review]
> > (part 9) - Use PLDHashTable2 in nsTHashtable
> 
> Well, this is odd. This patch works fine on all platforms but Windows, where
> it lights up Treeherder like a Skittles commercial:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4186e372a1c8

I did some local Windows builds and I've determined that it's the movement of |sOps| from a function to the class that's the problem; it's *not* the change of |mTable| to a PLDHashTable2. Specifically, Firefox builds with the |sOps| change simply fail to start, and debug builds give this assertion failure:

> Assertion failure: !GetModuleHandleA("mozglue.dll"), at c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\toolkit\\xre\\WindowsCrtPatch.h:126

WindowsCrtPatch.h is one of those special super hacks we have, and I don't claim to understand it at all. But somehow, having a templatized static class member is breaking it.

dmajor, any ideas why? Thank you.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df1d66a31697
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eaac66f8bbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe49838e8aba
https://hg.mozilla.org/integration/mozilla-inbound/rev/b373d9cab11f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9cc200b499
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b79df6cb61
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9eaaca25256
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5226ab4feca

Comment 22

[Nicholas Nethercote [inactive]]

I landed the first 8 patches.

> I did some local Windows builds and I've determined that it's the movement of |sOps| from a function
> to the class that's the problem

It looks like if I move |sOps| into a static function, that's enough to get past the WindowsCrtPatch.h problem. But I'll do that in another bug.

Comment 23

[Nicholas Nethercote [inactive]]

> Can you file followup bugs for:
> 
> a) adding the same MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING to the macros in
> nsAgg.h;
> b) making NS_GENERIC_AGGREGATED_CONSTRUCTOR not require access to the
> destructor?  (I think we can do this by use the same idea as
> NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT.).

Done: bug 1168006.

Comment 24

[(Away)]

> > Assertion failure: !GetModuleHandleA("mozglue.dll"), at c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\toolkit\\xre\\WindowsCrtPatch.h:126

Yikes! I'm glad the asserts caught this, and thanks for flagging me. Even if you've found a workaround, I'd like to build this locally tomorrow and see what's going on.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/df1d66a31697
https://hg.mozilla.org/mozilla-central/rev/7eaac66f8bbe
https://hg.mozilla.org/mozilla-central/rev/fe49838e8aba
https://hg.mozilla.org/mozilla-central/rev/b373d9cab11f
https://hg.mozilla.org/mozilla-central/rev/5a9cc200b499
https://hg.mozilla.org/mozilla-central/rev/d2b79df6cb61
https://hg.mozilla.org/mozilla-central/rev/b9eaaca25256
https://hg.mozilla.org/mozilla-central/rev/e5226ab4feca

Comment 26

[(Away)]

> Assertion failure: !GetModuleHandleA("mozglue.dll"), at c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\toolkit\\xre\\WindowsCrtPatch.h:126

For some reason part 9 was provoking the issue in bug 1023941 comment 32. I don't know why the xpcom directory is so susceptible to this. I tried to investigate this once before, in bug 1159416 comment 4, but I didn't get anywhere.