Last Comment Bug 474505 - Replace uses of nsVoidPtrHashkey with nsPtrHashKey<T>
: Replace uses of nsVoidPtrHashkey with nsPtrHashKey<T>
Status: RESOLVED FIXED
[good first bug] [mentor=ehsan]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Hessam Salehi
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-20 14:37 PST by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-03-22 06:29 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first patch (42.27 KB, patch)
2011-09-27 11:20 PDT, Hessam Salehi
no flags Details | Diff | Splinter Review
first patch (FIXED) (30.45 KB, patch)
2011-09-28 12:34 PDT, Hessam Salehi
ehsan: feedback+
Details | Diff | Splinter Review
patch v3 (31.53 KB, patch)
2011-10-01 14:25 PDT, Hessam Salehi
no flags Details | Diff | Splinter Review
Updated patch on top of last week's PRBool->bool change (30.51 KB, patch)
2011-10-03 12:44 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
patch v4 (30.96 KB, patch)
2012-03-14 13:40 PDT, Hessam Salehi
ehsan: feedback+
Details | Diff | Splinter Review
patch v5 (30.96 KB, patch)
2012-03-15 13:31 PDT, Hessam Salehi
no flags Details | Diff | Splinter Review
patch v6 (30.96 KB, patch)
2012-03-16 05:26 PDT, Hessam Salehi
benjamin: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-20 14:37:28 PST
We can stop using nsVoidPtrHashkey and start using the typed version.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-20 14:38:22 PST
When this is complete, we should just remove nsVoidPtrHashKey.
Comment 2 daniel newton 2009-01-22 16:43:41 PST
but nsVoidPtrHashkey is an nsPtrHashKey<T>?
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsHashKeys.h#248
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-22 17:32:53 PST
Yeah, but we shouldn't be using it since it gives us less typechecking and requires more type casts.
Comment 4 Josh Matthews [:jdm] 2011-09-16 11:16:14 PDT
I think it's safe to say that this is not being worked on at present.
Comment 5 :Ehsan Akhgari 2011-09-20 15:21:06 PDT
I'm stealing the mentorship from jdm on this one (hope you don't mind Josh!).
Comment 6 Hessam Salehi 2011-09-21 09:12:22 PDT
I'm working on this . I'll try to make a patch for all files , Of course because this is my first contribute to bugzilla I might I can not fix all the files correctly. However I will try.
Comment 7 Hessam Salehi 2011-09-27 11:20:32 PDT
Created attachment 562820 [details] [diff] [review]
first patch
Comment 8 Josh Matthews [:jdm] 2011-09-27 12:04:37 PDT
Comment on attachment 562820 [details] [diff] [review]
first patch

Thanks very much Hessam, this is looking really good. I'm not sure if |const void| is necessary in all of those changes rather than just |void|, but I'm sure Ehsan will know. It's great to see a bunch of these turning into real types instead of void pointers, too :)

Do you know what the deal is with those huge blocks of changes in nsDocument that don't seem to make any actual changes? Did you make any modifications in there? If not, can you just revert that file and make the other changes again?

>-  array->AppendElement(static_cast<nsSVGRenderingObserver*>(
>-          const_cast<void*>(aEntry->GetKey())));
>+  aEntry->GetKey();

This change is incorrect, since you lose the array->AppendElement.

Other than that, this patch looks promising! Good work!
Comment 9 Hessam Salehi 2011-09-28 12:34:37 PDT
Created attachment 563143 [details] [diff] [review]
first patch (FIXED)
Comment 10 Hessam Salehi 2011-09-28 12:54:01 PDT
Thank you for reviewing my patch .
I have corrected the NsDocument file and array->AppendElement according to your comment. about converting |const void| to |void| I'm waiting for ehsan comment .
Comment 11 Josh Matthews [:jdm] 2011-09-28 12:58:11 PDT
Comment on attachment 563143 [details] [diff] [review]
first patch (FIXED)

Presumably Ehsan does not have the clearance to review every single one of these changes, but I'll let him deal with that.
Comment 12 :Ehsan Akhgari 2011-09-28 17:29:15 PDT
Comment on attachment 563143 [details] [diff] [review]
first patch (FIXED)

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

This looks really good overall!  Thanks a lot for working on this.  I've made some suggestions on places where we could possibly use real types instead of void.  If you can fix those please, we can submit this for the final review round!  :-)

::: accessible/src/base/nsAccCache.h
@@ +65,5 @@
>   * Clear the cache and shutdown the accessibles.
>   */
>  template <class T>
>  static void
> +ClearCache(nsRefPtrHashtable<nsPtrHashKey<const void>, T> & aCache)

I think the only hashtable passed to this is nsAccessibleHashtable (defined in nsAccessible.h).  Can you make this a non-template function which takes an nsAccessibleHashtable& parameter?  It would definitely make this easier to read.

@@ +93,5 @@
>   * Traverse the accessible cache for cycle collector.
>   */
>  template <class T>
>  static void
> +CycleCollectorTraverseCache(nsRefPtrHashtable<nsPtrHashKey<const void>, T> & aCache,

Same here.

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +732,5 @@
>  
>    TurnOffNewTabSwitchingForJawsAndWE();
>  }
>  
> +nsRefPtrHashtable<nsPtrHashKey<void>, nsDocAccessible> nsAccessNodeWrap::sHWNDCache;

We should be able to use nsPtrHashKey<HWND> here, right?  And then we should be able to get rid of the void* cast further down in nsAccessNodeWrap::WindowProc when calling GetWeak on the hashtable.

::: accessible/src/msaa/nsAccessNodeWrap.h
@@ +169,5 @@
>  
>    static LRESULT CALLBACK WindowProc(HWND hWnd, UINT Msg,
>                                       WPARAM WParam, LPARAM lParam);
>  
> +  static nsRefPtrHashtable<nsPtrHashKey<void>, nsDocAccessible> sHWNDCache;

Same here, we should be able to use HWND.

::: content/base/src/nsDocument.h
@@ +1126,5 @@
>    PRUint8 mXMLDeclarationBits;
>  
>    PRUint8 mDefaultElementType;
>  
> +  nsInterfaceHashtable<nsPtrHashKey<nsIContent>, nsPIBoxObject> *mBoxObjectTable;

This looks very good, it's the correct type that we want to use as the hashtable key.

::: content/xbl/src/nsBindingManager.cpp
@@ +1331,5 @@
>  
>    return NS_OK;
>  }
>  
> +typedef nsTHashtable<nsPtrHashKey<const void> > RuleProcessorSet;

I think we can just use nsPtrHashKey<nsIStyleRuleProcessor>, and get rid of the related casts down the file.

The same goes for the other two places in this file which you've edited.

::: dom/base/nsGlobalWindow.h
@@ +964,5 @@
>    PRBool mCleanedUp, mCallCleanUpAfterModalDialogCloses;
>  
>    nsCOMPtr<nsIDOMOfflineResourceList> mApplicationCache;
>  
> +  nsDataHashtable<nsPtrHashKey<nsXBLPrototypeHandler>, void*> mCachedXBLPrototypeHandlers;

This looks very good!

::: dom/ipc/TabChild.cpp
@@ +373,5 @@
>      win.forget(aReturn);
>      return NS_OK;
>  }
>  
> +static nsInterfaceHashtable<nsPtrHashKey<void>, nsIDialogParamBlock> gActiveDialogs;

You should be able to use nsPtrHashKey<ContentDialogChild>.

::: dom/plugins/ipc/PluginInstanceParent.h
@@ +333,5 @@
>      const NPNetscapeFuncs* mNPNIface;
>      NPWindowType mWindowType;
>      int mQuirks;
>  
> +    nsDataHashtable<nsPtrHashKey<NPObject>, PluginScriptableObjectParent*> mScriptableObjects;

Looks great!

::: dom/plugins/ipc/PluginModuleParent.h
@@ +315,5 @@
>      bool mShutdown;
>      bool mClearSiteDataSupported;
>      bool mGetSitesWithDataSupported;
>      const NPNetscapeFuncs* mNPNIface;
> +    nsDataHashtable<nsPtrHashKey<void>, PluginIdentifierParent*> mIdentifiers;

You should be able to use nsPtrHashKey<NPIdentifier>.

::: dom/src/storage/nsDOMStorage.h
@@ +83,5 @@
>  using mozilla::dom::StorageParent;
>  
>  class DOMStorageImpl;
>  
> +class nsDOMStorageEntry : public nsPtrHashKey<const void>

Hmm, we could probably get rid of this class altogether and just use nsPtrHashKey<DOMStorageImpl> instead of it, but I'm not sure if that's going to work, and we don't need to handle that in this bug.  This looks good enough for now.

::: dom/src/threads/nsDOMThreadService.h
@@ +187,5 @@
>    // mCreationsInProgress.
>    mozilla::ReentrantMonitor mReentrantMonitor;
>  
>    // A map from nsDOMWorkerThread to nsDOMWorkerRunnable.
> +  nsRefPtrHashtable<nsPtrHashKey<nsDOMWorker>, nsDOMWorkerRunnable> mWorkersInProgress;

These look great!

::: dom/workers/RuntimeService.h
@@ +94,5 @@
>    // Protected by mMutex.
>    nsTArray<IdleThreadInfo> mIdleThreadArray;
>  
>    // *Not* protected by mMutex.
> +  nsClassHashtable<nsPtrHashKey<nsPIDOMWindow>, nsTArray<WorkerPrivate*> > mWindowMap;

This one is also great!

::: gfx/thebes/GLContext.h
@@ +1072,5 @@
>      // while preserving current ClearColor etc. values.
>      // Useful for resizing offscreen buffers.
>      void ClearSafely();
>  
> +    nsDataHashtable<nsPtrHashKey<void>, void*> mUserData;

This one looks good.  We're actually using void* pointers to key the hashtable!

::: js/src/xpconnect/src/xpcprivate.h
@@ +602,5 @@
>      PLDHashTable             mJSRoots;
>  #endif
>      nsAutoPtr<XPCCallContext> mCycleCollectionContext;
>  
> +    typedef nsBaseHashtable<nsPtrHashKey<const void>, nsISupports*, nsISupports*> ScopeSet;

Looks good.  We _could_ go with void instead of const void, but I don't care much!

::: layout/base/nsPresContext.h
@@ +1057,5 @@
>    // a specific language, however (e.g, if it is inferred from the
>    // charset rather than explicitly specified as a lang attribute).
>    nsIAtom*              mLanguage;      // [STRONG]
>  
> +  nsRefPtrHashtable<nsPtrHashKey<nsIFrame>, nsImageLoader>

This is very good!

::: layout/svg/base/src/nsSVGEffects.h
@@ +268,5 @@
>     */
>    void RemoveAll();
>  
>  private:
> +  nsTHashtable<nsPtrHashKey<nsSVGRenderingObserver> > mObservers;

Awesome!

::: layout/svg/base/src/nsSVGOuterSVGFrame.h
@@ +167,5 @@
>  
>    // A hash-set containing our nsSVGForeignObjectFrame descendants. Note we use
>    // a hash-set to avoid the O(N^2) behavior we'd get tearing down an SVG frame
>    // subtree if we were to use a list (see bug 381285 comment 20).
> +  nsTHashtable<nsPtrHashKey<nsSVGForeignObjectFrame> > mForeignObjectHash;

Super!

::: toolkit/components/remote/nsGTKRemoteService.h
@@ +79,5 @@
>  
>    virtual void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
>                                                PRUint32 aTimestamp);
>  
> +  nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;

This is very good!

::: xpcom/base/nsCycleCollector.cpp
@@ +732,5 @@
>  };
>  
>  // XXX Would be nice to have an nsHashSet<KeyType> API that has
>  // Add/Remove/Has rather than PutEntry/RemoveEntry/GetEntry.
> +typedef nsTHashtable<nsPtrHashKey<const void> > PointerSet;

As said before, we _could_ go with void here too, but I don't care much!

::: xpcom/threads/nsThreadManager.h
@@ +87,5 @@
>    }
>    
>    static nsThreadManager sInstance;
>  
> +  nsRefPtrHashtable<nsPtrHashKey<PRThread>, nsThread> mThreadsByPRThread;

Awesome!
Comment 13 Hessam Salehi 2011-10-01 14:25:45 PDT
Created attachment 563992 [details] [diff] [review]
patch v3

Thanks for reviewing my patch Ehsan. I fixed all the issues mentioned above .
Comment 14 :Ehsan Akhgari 2011-10-03 12:36:21 PDT
Comment on attachment 563992 [details] [diff] [review]
patch v3

I think this is ready for review now!
Comment 15 :Ehsan Akhgari 2011-10-03 12:44:48 PDT
Created attachment 564299 [details] [diff] [review]
Updated patch on top of last week's PRBool->bool change
Comment 16 :Ehsan Akhgari 2011-10-03 12:51:57 PDT
I've submitted this patch to the try server: http://tbpl.mozilla.org/?tree=Try&rev=f9042a973e06
Comment 17 Mozilla RelEng Bot 2011-10-03 17:31:30 PDT
Try run for f9042a973e06 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f9042a973e06
Results (out of 79 total builds):
    exception: 2
    success: 66
    warnings: 2
    failure: 9
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f9042a973e06
Comment 18 :Ehsan Akhgari 2011-10-03 20:35:47 PDT
Hesam, seems like there are two compilation problems with the patch.

The first one happens on Linux.  See this log for example: <https://tbpl.mozilla.org/php/getParsedLog.php?id=6658104&tree=Try&full=1>, which seems to have been caused because you haven't handled this case: <http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceParent.cpp#1423>.

The second one happens on Windows.  See this log for example: <https://tbpl.mozilla.org/php/getParsedLog.php?id=6658489&tree=Try&full=1>.  This one seems to be my fault.  :-)

I recommended you to do this:

-nsRefPtrHashtable<nsVoidPtrHashKey, nsDocAccessible> nsAccessNodeWrap::sHWNDCache;
+nsRefPtrHashtable<nsPtrHashKey<HWND>, nsDocAccessible> nsAccessNodeWrap::sHWNDCache;

HWND on Windows is defined as a pointer to a structure, something similar to this:

struct HWND__ {
  int dummy;
};
typedef HWND__* HWND;

Using nsPtrHashKey<HWND> makes the KeyType HWND* (or, HWND__**), not HWND.  That was my mistake.  I think you should go back to using nsPtrHashKey<void> (I'm very sorry about the wrong comment.)

Can you please submit a new patch addressing these two issues?

Thanks!
Comment 19 Benjamin Smedberg [:bsmedberg] 2011-10-07 09:40:37 PDT
Comment on attachment 563992 [details] [diff] [review]
patch v3

Clearing review request because of tryserver build bustage. I'll be happy to review a revised version!
Comment 20 Hessam Salehi 2012-03-14 13:40:27 PDT
Created attachment 605933 [details] [diff] [review]
patch v4

It's a little bit late, Sorry. :)
I have updated the patch so that it works with the latest updated source code, at least on my machine, Please submit this patch to the try server again. thanks.
Comment 21 Mozilla RelEng Bot 2012-03-14 14:04:32 PDT
Autoland Patchset:
	Patches: 564299, 605933
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Comment 22 :Ehsan Akhgari 2012-03-14 14:09:21 PDT
Comment on attachment 605933 [details] [diff] [review]
patch v4

Marking f+ to satisfy the autoland bot...
Comment 23 Mozilla RelEng Bot 2012-03-14 14:10:50 PDT
Autoland Patchset:
	Patches: 605933
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Comment 24 :Ehsan Akhgari 2012-03-14 14:13:44 PDT
Pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=b332b544c583
Comment 25 Mozilla RelEng Bot 2012-03-14 22:17:10 PDT
Try run for b332b544c583 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b332b544c583
Results (out of 113 total builds):
    success: 87
    warnings: 17
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-b332b544c583
Comment 26 :Ehsan Akhgari 2012-03-15 08:52:31 PDT
(In reply to Mozilla RelEng Bot from comment #25)
>     https://tbpl.mozilla.org/?tree=Try&rev=b332b544c583

Looking at the build failures here, it seems like there is a usage of nsVoidPtrHashKey in dom/plugins/ipc/PluginInstanceParent.h/cpp (see the mScriptableObjects member.)

Can you please fix that as well, Hessam?  Thanks!
Comment 27 Hessam Salehi 2012-03-15 13:31:56 PDT
Created attachment 606340 [details] [diff] [review]
patch v5

PluginInstanceParent and nsAccessNodeWrap fixed.
Comment 28 :Ehsan Akhgari 2012-03-15 13:49:00 PDT
http://tbpl.mozilla.org/?tree=Try&rev=d83deb02e6ee
Comment 29 Mozilla RelEng Bot 2012-03-15 18:46:46 PDT
Try run for d83deb02e6ee is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d83deb02e6ee
Results (out of 169 total builds):
    exception: 1
    success: 134
    warnings: 31
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-d83deb02e6ee
Comment 30 Hessam Salehi 2012-03-16 05:26:43 PDT
Created attachment 606541 [details] [diff] [review]
patch v6
Comment 31 :Ehsan Akhgari 2012-03-16 15:24:29 PDT
http://tbpl.mozilla.org/?tree=Try&rev=8faf0e760f9d
Comment 32 Mozilla RelEng Bot 2012-03-16 19:47:12 PDT
Try run for 8faf0e760f9d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8faf0e760f9d
Results (out of 216 total builds):
    exception: 3
    success: 172
    warnings: 27
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-8faf0e760f9d
Comment 33 :Ehsan Akhgari 2012-03-17 11:09:58 PDT
Comment on attachment 606541 [details] [diff] [review]
patch v6

This version of the patch compiles fine on all platforms.
Comment 34 :Ehsan Akhgari 2012-03-21 11:08:51 PDT
Thanks a lot for your patch, Hessam!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6cb28e6c61c
Comment 35 Marco Bonardo [::mak] 2012-03-22 06:29:32 PDT
https://hg.mozilla.org/mozilla-central/rev/a6cb28e6c61c

Note You need to log in before you can comment on or make changes to this bug.