Closed
Bug 910989
Opened 11 years ago
Closed 11 years ago
nsTHashtable::Init should go away
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(3 files)
18.20 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
367.88 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
31.64 KB,
patch
|
jcranmer
:
review+
standard8
:
checkin+
|
Details | Diff | Splinter Review |
It has no reason to exist. Most users of nsTHashtable are infallible now (bug 734847), and most don't pass an initial size. For those that do, we can supply constructors with the right form. Fallibly-constructed nsTHashtables can be checked for validity via IsInitialized.
Comment 1•11 years ago
|
||
Thanks for doing this! :-)
Assignee | ||
Comment 2•11 years ago
|
||
In fact on Linux, Firefox builds with support for fallible construction completely removed! I'll see if that's true on all platforms... It also builds with nsBaseHashtableMT and nsInterfaceHashtableMT removed.
Assignee | ||
Comment 3•11 years ago
|
||
There are a number of places that lazily initialize nsTHashtables, and sometimes the initialization status (via IsInitialized) as some kind of state flag. In some cases (e.g. singleton services) I'm just eagerly initializing the hashtable since there's no plausible way the increased memory usage could be significant. In others I'm wrapping the hashtable in nsAutoPtr and heap-allocating it at the point where it would have been lazily initialized.
Assignee | ||
Comment 4•11 years ago
|
||
These are the changes to the hashtable classes themselves.
Attachment #798336 -
Flags: review?(benjamin)
Assignee | ||
Comment 5•11 years ago
|
||
Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=bc0824540d92
Assignee | ||
Comment 6•11 years ago
|
||
Fix for Android build bustage here: https://tbpl.mozilla.org/?tree=Try&rev=51df6d81d06d
Assignee | ||
Comment 7•11 years ago
|
||
Do you think I should break this up for individual module owners to look at?
Attachment #798406 -
Flags: review?(ehsan)
Assignee | ||
Comment 8•11 years ago
|
||
The B2G R10 bustage from comment #5 was caused by an unrelated patch, so this patch is currently green.
Comment 9•11 years ago
|
||
Comment on attachment 798406 [details] [diff] [review] overall patch Review of attachment 798406 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/windows/msaa/nsWinUtils.cpp @@ +59,5 @@ > if (Compatibility::IsJAWS() || Compatibility::IsWE() || > Compatibility::IsDolphin() || > Preferences::GetBool("browser.tabs.remote")) { > RegisterNativeWindow(kClassNameTabContent); > + sHWNDCache = new nsRefPtrHashtable<nsPtrHashKey<void>, DocAccessible>(4); I assume we're not getting leak reports for not deleting this pointer? ::: extensions/spellcheck/hunspell/src/mozHunspell.cpp @@ +18,5 @@ > * Contributor(s): Kevin Hendricks (kevin.hendricks@sympatico.ca) > * David Einstein (deinst@world.std.com) > * Michiel van Leeuwen (mvl@exedo.nl) > * Caolan McNamara (cmc@openoffice.org) > + * L�szl� N�meth (nemethl@gyorsposta.hu) Please restore this hunk! ::: xpcom/glue/nsBaseHashtable.h @@ +60,5 @@ > > + nsBaseHashtable() > + { > + } > + nsBaseHashtable(uint32_t aInitSize) Please make this explicit. ::: xpcom/glue/nsClassHashtable.h @@ +29,5 @@ > > + nsClassHashtable() > + { > + } > + nsClassHashtable(uint32_t aInitSize) Ditto. ::: xpcom/glue/nsDataHashtable.h @@ +24,5 @@ > +public: > + nsDataHashtable() > + { > + } > + nsDataHashtable(uint32_t aInitSize) And this too. ::: xpcom/glue/nsInterfaceHashtable.h @@ +30,5 @@ > > + nsInterfaceHashtable() > + { > + } > + nsInterfaceHashtable(uint32_t aInitSize) Need I say more? :) ::: xpcom/glue/nsRefPtrHashtable.h @@ +29,5 @@ > > + nsRefPtrHashtable() > + { > + } > + nsRefPtrHashtable(uint32_t aInitSize) :) ::: xpcom/glue/nsTHashtable.h @@ +88,5 @@ > + nsTHashtable() > + { > + Init(PL_DHASH_MIN_SIZE); > + } > + nsTHashtable(uint32_t aInitSize) ... etc
Attachment #798406 -
Flags: review?(ehsan) → review+
Comment 10•11 years ago
|
||
(In reply to comment #7) > Created attachment 798406 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=798406&action=edit > overall patch > > Do you think I should break this up for individual module owners to look at? No, I don't think that would be valuable.
Comment 11•11 years ago
|
||
But it's probably worth writing to dev.platform about this when it lands.
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9) > ::: accessible/src/windows/msaa/nsWinUtils.cpp > @@ +59,5 @@ > > if (Compatibility::IsJAWS() || Compatibility::IsWE() || > > Compatibility::IsDolphin() || > > Preferences::GetBool("browser.tabs.remote")) { > > RegisterNativeWindow(kClassNameTabContent); > > + sHWNDCache = new nsRefPtrHashtable<nsPtrHashKey<void>, DocAccessible>(4); > > I assume we're not getting leak reports for not deleting this pointer? Not on try at least. > Please restore this hunk! Oops. > ::: xpcom/glue/nsBaseHashtable.h > @@ +60,5 @@ > > > > + nsBaseHashtable() > > + { > > + } > > + nsBaseHashtable(uint32_t aInitSize) > > Please make this explicit. Good point.
Updated•11 years ago
|
Attachment #798336 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc427f5ec61b
Assignee | ||
Comment 14•11 years ago
|
||
Somehow I searched .cpp files in comm-central for 'Hashtable', didn't find any hits, and concluded these types weren't used. They are, of course.
Attachment #800122 -
Flags: review?(Pidgeot18)
Comment 15•11 years ago
|
||
Comment on attachment 800122 [details] [diff] [review] comm-central patch >- m_pseudoHdrs.Init(10); jcranmer asked me to have a quick look at this, it all seems fine except this one line which seems to suggest that a non-default constructor be used.
Comment 16•11 years ago
|
||
Comment on attachment 800122 [details] [diff] [review] comm-central patch Review of attachment 800122 [details] [diff] [review]: ----------------------------------------------------------------- r+, module Neil's concern. I see some stuff that looks broken here, but none of it has to do with the hashtable stuff.
Attachment #800122 -
Flags: review?(Pidgeot18) → review+
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc427f5ec61b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee | ||
Comment 18•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #15) > >- m_pseudoHdrs.Init(10); > jcranmer asked me to have a quick look at this, it all seems fine except > this one line which seems to suggest that a non-default constructor be used. Setting the bucket size is a microoptimization that is unlikely to make any observable difference. I took this one out because it was slightly annoying to fix.
Assignee | ||
Updated•11 years ago
|
Attachment #800122 -
Flags: checkin?
Comment 19•11 years ago
|
||
Comment on attachment 800122 [details] [diff] [review] comm-central patch https://hg.mozilla.org/comm-central/rev/ec3cd803c556
Attachment #800122 -
Flags: checkin? → checkin+
You need to log in
before you can comment on or make changes to this bug.
Description
•