Closed
Bug 1161377
Opened 9 years ago
Closed 9 years ago
PLDHashTable: Add and use an initializing constructor and destructor
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
WONTFIX
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(4 files)
11.42 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
18.47 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
21.58 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
49.87 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
They're not needed now that there is an initializing constructor and a destructor.
Attachment #8601896 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8601897 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•9 years ago
|
||
> > + 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8e382a1e45 https://hg.mozilla.org/integration/mozilla-inbound/rev/2eae1608bcfa https://hg.mozilla.org/integration/mozilla-inbound/rev/c375efe78e07
https://hg.mozilla.org/mozilla-central/rev/3d8e382a1e45 https://hg.mozilla.org/mozilla-central/rev/2eae1608bcfa https://hg.mozilla.org/mozilla-central/rev/c375efe78e07
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(nfroyd)
Comment 10•9 years ago
|
||
(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)
Assignee | ||
Comment 11•9 years ago
|
||
> 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.
Assignee | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6fd4dfe3a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0587959d25e
https://hg.mozilla.org/mozilla-central/rev/6f6fd4dfe3a3 https://hg.mozilla.org/mozilla-central/rev/a0587959d25e
Assignee | ||
Comment 18•9 years ago
|
||
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.
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → WONTFIX
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
> 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?
Comment 21•9 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•