Closed Bug 910989 Opened 11 years ago Closed 11 years ago

nsTHashtable::Init should go away

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(3 files)

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.
Thanks for doing this! :-)
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.
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.
These are the changes to the hashtable classes themselves.
Attachment #798336 - Flags: review?(benjamin)
Attached patch overall patchSplinter Review
Do you think I should break this up for individual module owners to look at?
Attachment #798406 - Flags: review?(ehsan)
The B2G R10 bustage from comment #5 was caused by an unrelated patch, so this patch is currently green.
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+
(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.
But it's probably worth writing to dev.platform about this when it lands.
(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.
Attachment #798336 - Flags: review?(benjamin) → review+
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 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 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+
https://hg.mozilla.org/mozilla-central/rev/bc427f5ec61b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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.
Depends on: 919403
Depends on: 955889
You need to log in before you can comment on or make changes to this bug.