211470 - A nsIURI-based hashtable key could be useful

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For example, we have code duplication between
http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULPrototypeCache.cpp#126
and
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSLoader.h#80

Comment 1

[Benjamin Smedberg]

Attachment: nsURIHashKey

There are a coupla issues with this:

1) hashing the string value of nsIURI does not do the same equality checks as
nsIURI.equals.	That's the way it was, so I haven't changed it, but a smarter
hashing mechanism might be nice.

2) CSSLoaderImpl has an Init(nsIDocument*) method, but in a coupla cases this
method was not being called (ua.css and the chrome cssloader). I discovered
these cases by crash ;) I hope that there aren't others that I missed. I just
changed these to call Init(nsnull).

3) one of the implementations cached the hashcode in the key class, rather than
hashing each time. I don't know the performance impacts of this, so I didn't do
it... if this appears to hurt performance, I can try caching the hashcode (it's
a three-line alteration).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127053 [details] [diff] [review]
nsURIHashKey

Will this key ever be allocated anywhere but on the stack?  If so, please add
some destructor/constructor counters for the refcount logging code to work
with.

>Index: content/html/style/src/nsCSSLoader.h
>+ * SheetLoadData -- a small class that is used to store all the
>  *                     information needed for the loading of a sheet;
>  *                     this class handles listening for the stream
>  *                     loader completion and also handles charset
>  *                     determination.

Please keep the indentation consistent in that comment, like it used to be.

> NS_IMETHODIMP
> CSSLoaderImpl::Init(nsIDocument* aDocument)
> {
>   NS_ASSERTION(! mDocument, "already initialized");
>+
>+  NS_ENSURE_TRUE(mCompleteSheets.Init(), NS_ERROR_OUT_OF_MEMORY);
>+  NS_ENSURE_TRUE(mLoadingDatas.Init(), NS_ERROR_OUT_OF_MEMORY);
>+  NS_ENSURE_TRUE(mPendingDatas.Init(), NS_ERROR_OUT_OF_MEMORY);
>+

I've never much liked this function being called Init(), and in fact I was
considering renaming it to SetDocument()....  Would it be possible to init the
hashtables in the constructor and check whether they are inited in the loading
methods (bailing out with an error if they are not inited)?

Alternately, we could have an Init() function on nsCSSLoader that both the
factory constructor and the NS_NewCSSLoader functions would call; then the
creators of CSSLoader don't need to worry about having to init it...

>-PR_STATIC_CALLBACK(PRIntn)
>-StartAlternateLoads(nsHashKey *aKey, void *aData, void* aClosure)
>+PLDHashOperator PR_CALLBACK
>+StartAlternateLoads(nsIURI *aKey, SheetLoadData* &aData, void* 
aClosure)

What's the rationale for not using PR_STATIC_CALLBACK(PLDHashOperator) ?

>+PLDHashOperator PR_CALLBACK
>+StartNonAlternates(nsIURI *aKey, SheetLoadData* &aData, void* aClosure)

Same thing.

Also, why is the second arg in both cases SheetLoadData*& instead of just
SheetLoadData* ?


>+      mCompleteSheets.Get(aURI, getter_AddRefs(sheet));
>+        mLoadingDatas.Get(aURI, &loadData);
>+          mPendingDatas.Get(aURI, &loadData);

Hmmm.... couldn't we declare an nsURIHashKey(aURI) on the stack here and pass
that instead of rehashing the URI every time?

>-#ifdef DEBUG
>-      SheetLoadData* removedData =
>-#endif
>-        NS_STATIC_CAST(SheetLoadData*, mPendingDatas.Remove(&key));
>-      NS_ASSERTION(removedData == existingData, "Broken pending table");
>+      mPendingDatas.Remove(aLoadData->mURI);

Could you keep the debug code please?  (use ->Get() inside the DEBUG ifdef to
get the |removedData|, I guess..  Or is there a good reason to remove that
code?

>+PLDHashOperator PR_CALLBACK
>+StopLoadingSheetCallback(nsIURI* aKey, SheetLoadData*& aData, void* aClosure)

Aagin, why not PR_STATIC_CALLBACK, and what's up with the second arg?

>Index: content/xul/document/src/nsXULPrototypeCache.cpp

>+    if (!(result->mPrototypeTable.Init() &&
>+          result->mStyleSheetTable.Init() &&
>+          result->mScriptTable.Init() &&
>+          result->mXBLDocTable.Init() &&
>+          result->mFastLoadURITable.Init())) {
>+        delete result;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }

Please put this check before the pref callback registration (and please use
"cvs diff -p" to make patches more readable...)

>     // Put() w/o  a third parameter with a destination for the
>     // replaced value releases it
>-    mPrototypeTable.Put(&key, aDocument);
>+    mPrototypeTable.Put(uri, aDocument);

Does that comment still have any bearing on reality?

> nsXULPrototypeCache::GetStyleSheet(nsIURI* aURI, nsICSSStyleSheet** _result)
> {
>-    nsIURIKey key(aURI);
>-    *_result = NS_STATIC_CAST(nsICSSStyleSheet*, mStyleSheetTable.Get(&key));
>+    mStyleSheetTable.Get(aURI,_result);

Space before "_result", please.

>+static PLDHashOperator PR_CALLBACK
>+UnlockJSObjectCallback(nsIURI* aKey, void* &aData, void* aClosure)

Again, why not PR_STATIC_CALLBACK?

> {
>     JS_UnlockGCThingRT((JSRuntime*) aClosure, aData);
>-    return PR_TRUE;
>+    return PL_DHASH_REMOVE;
> }

Hmm... This now not only unlocks the JS objects, it also removes them from the
hashtable.  Should the name be changed accordingly?

>     nsCAutoString str;
>     uri->GetSpec(str);
>+    nsCOMPtr<nsIXBLDocumentInfo> info;
>+    mXBLDocTable.Get(str, getter_AddRefs(info));

It looks like users of mXBLDocTable and callers of those functions all
ultimately get the CString objects from an nIURI via GetSpec.  Please file a
follow-up bug on maybe converting this to be an nsIURI key?

>+PLDHashOperator PR_CALLBACK
>+FlushSkinXBL(const nsACString& key, nsCOMPtr<nsIXBLDocumentInfo>& aDocInfo, void* aClosure)

PR_STATIC_CALLBACK, maybe?  Is the second arg passed in by the hashtable code
really a reference to an nsCOMPtr?  Why not just a raw pointer?  The latter
would make more sense, imo...

>+PLDHashOperator PR_CALLBACK
>+FlushSkinSheets(nsIURI* aKey, nsCOMPtr<nsICSSStyleSheet>& aSheet, void* aClosure)

Same.

>+PLDHashOperator PR_CALLBACK
>+FlushScopedSkinStylesheets(const nsACString& aKey, nsCOMPtr<nsIXBLDocumentInfo> &aDocInfo, void* aClosure)

Same.

>+        mFastLoadURITable.Put(aURI, 1);
>+    mFastLoadURITable.Put(aURI, 1);

Again, could we only create the key once?

I agree with you about hashcode computation... we can improve the hash key
class if needed, as long as all callers use it sanely.

And yes, we may want to make the hashcode computation smarter (as regards case,
etc).  Not quite clear how we would do that (and not this bug in any case).

Comment 3

[Benjamin Smedberg]

> Will this key ever be allocated anywhere but on the stack?
Actually, it is never allocated on the stack, only within the hashtable.

> considering renaming it to SetDocument()....  Would it be possible to init the
> hashtables in the constructor and check whether they are inited in the loading
> methods (bailing out with an error if they are not inited)?
I can change it to if (!(table.IsInitialized() || table.Init()))

> Also, why is the second arg in both cases SheetLoadData*& instead of just
> SheetLoadData* ?
the Enumerate callback allows you to change the data stored in the hashtable
through the reference. Same thing later on with the COMPtr

> Could you keep the debug code please?  (use ->Get() inside the DEBUG ifdef to
> get the |removedData|, I guess..  Or is there a good reason to remove that
> code?
Will use Get()  I changed it because the Remove method doesn't return the
previous value.

> Hmmm.... couldn't we declare an nsURIHashKey(aURI) on the stack here and pass
> that instead of rehashing the URI every time?
No. In order to bypass the vtable used by nsHashtable, you actually pass in the
key itself, rather than an arbitrary key class.

I am getting married on Saturday, so I won't be able to actually post a new
patch until 23-July at the earliest.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> the Enumerate callback allows you to change the data stored in the hashtable
> through the reference.

Is there a callback that does not (EnumerateRead seems like it may do the
trick)?  Could we overload Enumerate to do different things depending on the
callback type passed in?

> you actually pass in the key itself, rather than an arbitrary key class.

Hmmm.  That is indeed what it looks like when I read the headers... but when I
did that security manager patch, I was passing in instances of the key class and
things compiled... what's up with that?  If that does not work, we should make
sure it does not compile...

Congrats on getting married!  Have fun, and I look forward to the updated patch
whenever life goes back to "normal" for you.  ;)

Comment 5

[Darin Fisher]

Comment on attachment 127053 [details] [diff] [review]
nsURIHashKey

i'll wait for the revised patch.

Comment 6

[Benjamin Smedberg]

> Will this key ever be allocated anywhere but on the stack?  If so, please add
> some destructor/constructor counters for the refcount logging code to work

Am I allowed to put the MOZ_COUNT_* macros in an inline constructor?

> Is there a callback that does not [use a reference] (EnumerateRead seems like
it may do the
> trick)?  Could we overload Enumerate to do different things depending on the
> callback type passed in?

EnumerateRead doesn't work, because it doesn't allow a PL_DHASH_REMOVE during
enumeration (which would be a non-const operation). I could overload Enumerate,
but I don't really want to... the class declaration is complicated enough as it is.

Comment 7

[Benjamin Smedberg]

Attachment: nsURIHashKey, take 2

take 2, nits picked except for the enumerator references, which I don't think
should be changed

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128497 [details] [diff] [review]
nsURIHashKey, take 2

>Index: content/html/style/src/nsCSSLoader.cpp

>@@ -1212,10 +1212,11 @@ CSSLoaderImpl::LoadSheet(SheetLoadData* 
>+  NS_ASSERTION(mLoadingDatas.IsInitialized(), "mLoadingDatas should be initialized by now.");

You probably want to assert about mPendingDatas too, here, since this method
uses it.

>@@ -1448,25 +1452,20 @@ void
> CSSLoaderImpl::SheetComplete(SheetLoadData* aLoadData, PRBool aSucceeded)
>-#ifdef DEBUG
>-      SheetLoadData* loadingData =
>-#endif
>-        NS_STATIC_CAST(SheetLoadData*, mLoadingDatas.Remove(&key));
>-      NS_ASSERTION(loadingData == aLoadData, "Broken loading table");
>+      mLoadingDatas.Remove(aLoadData->mURI);
>       aLoadData->mIsLoading = PR_FALSE;

Again, please leave the debug code...

sr=bzbarsky with those nits.

Comment 9

[Darin Fisher]

Comment on attachment 128497 [details] [diff] [review]
nsURIHashKey, take 2

>Index: netwerk/base/public/nsNetUtil.h

>+ *   Bradley Baetz <[e-mail]>
>+ *   Benjamin Smedberg <[e-mail]>

maybe a good idea to fix bbaetz's email address?  ([e-mail])


>+/**
>+ * Hashtable key class to use with nsTHashtable.
>+ */
>+MOZ_DECL_CTOR_COUNTER(nsURIHashKey)

so this macro doesn't seem to do anything... is it really needed?


>+class nsURIHashKey : public PLDHashEntryHdr

i think it'd be better to move this class definition into a separate
header file: netwerk/base/public/nsURIHashKey.h
...that way folks can choose to include only the header files they need
instead of being forced to include all of nsNetUtil.h just for this
class.


>Index: content/html/style/src/nsCSSLoader.h

>+#include "nsNetUtil.h"
>+#include "nsInterfaceHashtable.h"
>+#include "nsDataHashtable.h"

e.g., notice that any change to nsNetUtil.h will now require a full
rebuild of anything that includes nsCSSLoader.h :(


>+  nsInterfaceHashtable<nsURIHashKey,nsICSSStyleSheet> mCompleteSheets;
>+  nsDataHashtable<nsURIHashKey,SheetLoadData*> mLoadingDatas; // weak refs
>+  nsDataHashtable<nsURIHashKey,SheetLoadData*> mPendingDatas; // weak refs

is it possible for these declarations to work without seeing the full
class definition?  i.e., would it be sufficient to just forward declare
class nsURIHashKey?  probably not, huh?


>Index: content/xul/document/src/nsXULPrototypeCache.cpp

>-    nsSupportsHashtable mPrototypeTable;
>-    nsSupportsHashtable mStyleSheetTable;
>-    nsHashtable         mScriptTable;
>-    nsSupportsHashtable mXBLDocTable;
>+    nsInterfaceHashtable<nsURIHashKey,nsIXULPrototypeDocument> mPrototypeTable;
>+    nsInterfaceHashtable<nsURIHashKey,nsICSSStyleSheet> mStyleSheetTable;
>+    nsDataHashtable<nsURIHashKey,void*> mScriptTable;
>+    nsInterfaceHashtable<nsCStringHashKey,nsIXBLDocumentInfo> mXBLDocTable;

style nit: variable names used to be lined up starting in the same column..
maybe
the author of this file likes it that way.  how about aligning all the variable
names with mPrototypeTable?


> NS_IMETHODIMP
> nsXULPrototypeCache::PutStyleSheet(nsICSSStyleSheet* aStyleSheet)
> {
>     nsresult rv;
>     nsCOMPtr<nsIURI> uri;
>     rv = aStyleSheet->GetURL(*getter_AddRefs(uri));
>+    NS_ENSURE_SUCCESS(rv, rv);
> 
>+    mStyleSheetTable.Put(uri, aStyleSheet);
>     return NS_OK;
> }

footprint nit: avoid duplication of code to call ~nsCOMPtr if possible.

  {
      nsresult rv;
      nsCOMPtr<nsIURI> uri;
      rv = aStyleSheet->GetURL(*getter_AddRefs(uri));
      if (NS_SUCCEEDED(rv))
	  mStyleSheetTable.Put(uri, aStyleSheet);
      else
	  NS_WARNING("GetURL failed");
      return rv;
  }


>+PR_STATIC_CALLBACK(PLDHashOperator)
>+FlushSkinXBL(const nsACString& key, nsCOMPtr<nsIXBLDocumentInfo>& aDocInfo, void* aClosure)
> {
>   nsCOMPtr<nsIDocument> doc;
>+  aDocInfo->GetDocument(getter_AddRefs(doc));
>   nsCOMPtr<nsIURI> uri;
>   doc->GetDocumentURL(getter_AddRefs(uri));
>   nsCAutoString str;
>   uri->GetPath(str);
>   if (!strncmp(str.get(), "/skin", 5)) {
>+    return PL_DHASH_REMOVE;
>   }
>+
>+  return PL_DHASH_NEXT;
> }

same footprint nit applies to this function.  because you have two different
return paths, most compilers will generate two separate sets of dtor calls,
which in the case of this function adds up.  it might be better to save the
return value into a local variable and then have only one return point.  it
would probably reduce the codesize of this function by ~30%, which really
adds up if you consider how many functions in mozilla are written like this.


>+FlushSkinSheets(nsIURI* aKey, nsCOMPtr<nsICSSStyleSheet>& aSheet, void* aClosure)

same footprint nit in this function


r=darin with nsURIHashKey.h and whatever nits you choose to pick ;-)

Comment 10

[Benjamin Smedberg]

checked in 2003-07-27