Closed Bug 1161377 Opened 9 years ago Closed 9 years ago

PLDHashTable: Add and use an initializing constructor and destructor

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(4 files)

Now that PLDHashTable initialization is infallible, we should allow it to be done via a constructor. And while I'm dragging this code into the land of modern C++, a destructor will be nice... though there will need to still be the option of manual init/finish due to the gnarliness of some of the existing uses.
The destructor is "opt-in" -- there's a flag that makes it a no-op unless the
table was initialized with the initializing constructor. This will allow us to
incrementally convert existing tables from manual to automatic
initialization/finalization. This is important because some of the existing
uses are tricky (impossible?) to convert to the automatic style.
Attachment #8601895 - Flags: review?(nfroyd)
They're not needed now that there is an initializing constructor and a
destructor.
Attachment #8601896 - Flags: review?(nfroyd)
This patch converts easy cases, i.e. where the PL_DHashTableInit() call occurs
in a constructor and the PL_DHashTableFinish() call occurs in a destructor.
Attachment #8601897 - Flags: review?(nfroyd)
Comment on attachment 8601895 [details] [diff] [review]
(part 1) - Add an initializing constructor and destructor to PLDHashTable

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

::: xpcom/glue/pldhash.cpp
@@ +276,5 @@
> +  , mRecursionLevel()
> +#endif
> +{
> +  Init(aOps, aEntrySize, aLength);
> +  mAutoFinish = 1;

= true?  (Since you use false in other places.)
Attachment #8601895 - Flags: review?(nfroyd) → review+
Comment on attachment 8601896 [details] [diff] [review]
(part 2) - Remove PL_NewDHashTable() and PL_DHashTableDestroy()

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

Please file a followup for using UniquePtr (preferred) or nsAutoPtr for things which we initialize $SOMEWHERE and then |delete| in the destructor.  Thanks.
Attachment #8601896 - Flags: review?(nfroyd) → review+
Attachment #8601897 - Flags: review?(nfroyd) → review+
> > +  mAutoFinish = 1;
> 
> = true?  (Since you use false in other places.)

I'll change the |false| instances to |0|, given that it's a uint32_t:1 field.
Blocks: 1163331
I backed out part 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd634573d29

because it (probably) caused an increase in the number of static constructors:

> Regression: Mozilla-Inbound - Number of Constructors - CentOS release 5 (Final) - 5.43% increase
> ------------------------------------------------------------------------------------------------
>     Previous: avg 92.000 stddev 0.000 of 12 runs up to revision fa8214db7c82
>     New     : avg 97.000 stddev 0.000 of 12 runs since revision c375efe78e07
>     Change  : +5.000 (5.43% / z=0.000)
>     Graph   : http://mzl.la/1JT6Ks1

And also caused a Fennec startup regression: bug 1163066.

I say "probably" because part 3 seems the most likely to have caused those.

froydnj, can you see why there are five new static constructors? I can imagine two -- sMapOps and sRequestInfoHashOps -- but am not sure where the others are from.
Depends on: 1163066
Flags: needinfo?(nfroyd)
(In reply to Nicholas Nethercote [:njn] from comment #9)
> I backed out part 3:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd634573d29
> 
> because it (probably) caused an increase in the number of static
> constructors:
> 
> > Regression: Mozilla-Inbound - Number of Constructors - CentOS release 5 (Final) - 5.43% increase
> > ------------------------------------------------------------------------------------------------
> >     Previous: avg 92.000 stddev 0.000 of 12 runs up to revision fa8214db7c82
> >     New     : avg 97.000 stddev 0.000 of 12 runs since revision c375efe78e07
> >     Change  : +5.000 (5.43% / z=0.000)
> >     Graph   : http://mzl.la/1JT6Ks1
> 
> And also caused a Fennec startup regression: bug 1163066.
> 
> I say "probably" because part 3 seems the most likely to have caused those.

Graphs suggest that this didn't fix anything.

http://graphs.mozilla.org/graph.html#tests=[[81,63,6]]&sel=1431205257117.7297,1431329213874.4866,0,98.90710382513662&displayrange=7&datatype=geo

Your backout is just after the (second) 22:00 mark.

> froydnj, can you see why there are five new static constructors? I can
> imagine two -- sMapOps and sRequestInfoHashOps -- but am not sure where the
> others are from.

sMapOps/sRequestInfoHashOps don't add static constructors, because PLDHashTableOps doesn't have a (non-trivial) constructor or destructor.  So those are fine.

It's more likely, I think, that part 1 is responsible for the constructors (and possibly the Fennec startup regression), since that part gives PLDHashTable a destructor.  And after that, if there are PLDHashTables at global scope (like gHashTable in prefapi.cpp) or PLDHashTable members of classes that have static instances at global scope (unsure of what these are, but there are probably four such static instances!), then we've just increased the need for static constructors.  (The static constructor is needed to register an at-exit--maybe an at-library-unload--call to the destructor.)
Flags: needinfo?(nfroyd)
> It's more likely, I think, that part 1 is responsible for the constructors
> (and possibly the Fennec startup regression), since that part gives
> PLDHashTable a destructor.  And after that, if there are PLDHashTables at
> global scope (like gHashTable in prefapi.cpp) or PLDHashTable members of
> classes that have static instances at global scope (unsure of what these
> are, but there are probably four such static instances!), then we've just
> increased the need for static constructors.  (The static constructor is
> needed to register an at-exit--maybe an at-library-unload--call to the
> destructor.)

Yes, that sounds right. Thank you for the clarification. I see *eight* static PLDHashTable instances:

netwerk/protocol/http/nsHttp.cpp:static PLDHashTable         sAtomTable;
parser/htmlparser/nsHTMLEntities.cpp:static PLDHashTable gEntityToUnicode;
parser/htmlparser/nsHTMLEntities.cpp:static PLDHashTable gUnicodeToEntity;
xpcom/ds/nsAtomTable.cpp:static PLDHashTable gAtomTable;
dom/plugins/base/nsJSNPRuntime.cpp:static PLDHashTable sNPObjWrappers;
dom/base/nsContentList.cpp:static PLDHashTable gContentListHashTable;
dom/base/nsContentList.cpp:static PLDHashTable gFuncStringContentListHashTable;
dom/base/nsContentUtils.cpp:static PLDHashTable sEventListenerManagersHash;

Not sure why that doesn't match the five, but who knows what the compiler is doing.

I could back out parts 1 and 2 (and then bug 1163331's patch would also need backing out; I'm getting sick of comm-central being separate from mozilla-central).

Alternatively I could just change all the |static PLDHashTable| instances to |static PLDHashTable*|. That's how we do things with static nsTHashtable instances.
This reduces the number of static constructors from 97 back to 92, where it was
before part 1's patch.
Attachment #8604472 - Flags: review?(nfroyd)
Comment on attachment 8604472 [details] [diff] [review]
(part 2.5) - Move all static PLDHashTable instances onto the heap to avoid static constructors

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

::: xpcom/ds/nsAtomTable.cpp
@@ +709,5 @@
>  
>  nsrefcnt
>  NS_GetNumberOfAtoms(void)
>  {
> +  return gAtomTable ? gAtomTable->EntryCount() : 0;

Hum, given the previous code, maybe EntryCount() should assert that the table has been initialized.  Perhaps that will be less of an issue once everything uses new/delete, and PLDHashTables will be pointer-based?
Attachment #8604472 - Flags: review?(nfroyd) → review+
Depends on: 1165155
I ended up backing these patches out due to bug 1163066 and bug 1165155:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9e4027b6f5

I'll try re-landing the changes in smaller pieces, with some tweaks, but in new bugs.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → WONTFIX
(In reply to Nicholas Nethercote [:njn] from comment #11)

> I'm getting sick of comm-central being separate from mozilla-central).
Us too. There was a plan to merge comm-central back into mozilla-central sometime in 2015 but someone seems to have forgotten his promises.
See Also: → 1165674
> There was a plan to merge comm-central back into mozilla-central
> sometime in 2015 but someone seems to have forgotten his promises.

In the last discussion I recall on this topic (on dev-platform, sometime in the past year) the idea was summarily dismissed. Has something changed?
(In reply to Nicholas Nethercote [:njn] from comment #20)
> > There was a plan to merge comm-central back into mozilla-central
> > sometime in 2015 but someone seems to have forgotten his promises.
> 
> In the last discussion I recall on this topic (on dev-platform, sometime in
> the past year) the idea was summarily dismissed. Has something changed?

Joshua would know more of the nuance, but IIRC bsmedberg wanted to wait until hg supported some feature (I believe it was partial checkouts) before considering this further. But last time we went down this path, Joshua did a huge amount of prep work only to have the idea rejected at the end, so that sorta kills motivation.
No longer blocks: 1163331
You need to log in before you can comment on or make changes to this bug.