From 2abd02743546b07541abaeb9cdeb0ef4ed73dc31 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 8 Jun 2010 15:58:26 -0400 Subject: [PATCH] Bug 564569. Speed up the cache-hit case of getElementsBy(Class)Name. r=jst --- content/base/src/nsContentList.cpp | 16 +++--- content/base/src/nsContentList.h | 23 ++++++-- content/base/src/nsContentUtils.cpp | 58 ++++++++++---------- content/html/document/src/nsHTMLDocument.cpp | 7 +++ content/html/document/src/nsHTMLDocument.h | 10 +--- 5 files changed, 66 insertions(+), 48 deletions(-) diff --git a/content/base/src/nsContentList.cpp b/content/base/src/nsContentList.cpp index 098519b04ff..2a1b57d862f 100644 --- a/content/base/src/nsContentList.cpp +++ b/content/base/src/nsContentList.cpp @@ -313,7 +313,7 @@ already_AddRefed NS_GetFuncStringContentList(nsINode* aRootNode, nsContentListMatchFunc aFunc, nsContentListDestroyFunc aDestroyFunc, - void* aData, + nsFuncStringContentListDataAllocator aDataAllocator, const nsAString& aString) { NS_ASSERTION(aRootNode, "content list has to have a root"); @@ -361,7 +361,14 @@ NS_GetFuncStringContentList(nsINode* aRootNode, if (!list) { // We need to create a ContentList and add it to our new entry, if // we have an entry - list = new nsCacheableFuncStringContentList(aRootNode, aFunc, aDestroyFunc, aData, aString); + list = new nsCacheableFuncStringContentList(aRootNode, aFunc, aDestroyFunc, + aDataAllocator, aString); + if (list && !list->AllocatedData()) { + // Failed to allocate the data + delete list; + list = nsnull; + } + if (entry) { if (list) entry->mContentList = list; @@ -370,11 +377,6 @@ NS_GetFuncStringContentList(nsINode* aRootNode, } NS_ENSURE_TRUE(list, nsnull); - } else { - // List was already in the hashtable; clean up our new aData - if (aDestroyFunc) { - (*aDestroyFunc)(aData); - } } NS_ADDREF(list); diff --git a/content/base/src/nsContentList.h b/content/base/src/nsContentList.h index b7741c9eb83..e056032c1a8 100644 --- a/content/base/src/nsContentList.h +++ b/content/base/src/nsContentList.h @@ -432,7 +432,7 @@ public: PRUint32 GetHash(void) const { return NS_PTR_TO_INT32(mRootNode) ^ (NS_PTR_TO_INT32(mFunc) << 12) ^ - nsCRT::HashCode(PromiseFlatString(mString).get()); + nsCRT::HashCode(mString.BeginReading(), mString.Length()); } private: @@ -443,16 +443,27 @@ private: const nsAString& mString; }; +/** + * A function that allocates the matching data for this + * FuncStringContentList. Returning aString is perfectly fine; in + * that case the destructor function should be a no-op. + */ +typedef void* (*nsFuncStringContentListDataAllocator)(nsINode* aRootNode, + const nsString* aString); + +// aDestroyFunc is allowed to be null class nsCacheableFuncStringContentList : public nsContentList { public: nsCacheableFuncStringContentList(nsINode* aRootNode, nsContentListMatchFunc aFunc, nsContentListDestroyFunc aDestroyFunc, - void* aData, + nsFuncStringContentListDataAllocator aDataAllocator, const nsAString& aString) : - nsContentList(aRootNode, aFunc, aDestroyFunc, aData), + nsContentList(aRootNode, aFunc, aDestroyFunc, nsnull), mString(aString) - {} + { + mData = (*aDataAllocator)(aRootNode, &mString); + } virtual ~nsCacheableFuncStringContentList(); @@ -460,6 +471,8 @@ public: return mRootNode == aKey->mRootNode && mFunc == aKey->mFunc && mString == aKey->mString; } + + PRBool AllocatedData() const { return !!mData; } protected: virtual void RemoveFromCaches() { RemoveFromFuncStringHashtable(); @@ -477,6 +490,6 @@ already_AddRefed NS_GetFuncStringContentList(nsINode* aRootNode, nsContentListMatchFunc aFunc, nsContentListDestroyFunc aDestroyFunc, - void* aData, + nsFuncStringContentListDataAllocator aDataAllocator, const nsAString& aString); #endif // nsContentList_h___ diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp index 0b529518c9e..59ebd2c67b8 100644 --- a/content/base/src/nsContentUtils.cpp +++ b/content/base/src/nsContentUtils.cpp @@ -5784,6 +5784,10 @@ MatchClassNames(nsIContent* aContent, PRInt32 aNamespaceID, nsIAtom* aAtom, // need to match *all* of the classes ClassMatchingInfo* info = static_cast(aData); PRInt32 length = info->mClasses.Count(); + if (!length) { + // If we actually had no classes, don't match. + return PR_FALSE; + } PRInt32 i; for (i = 0; i < length; ++i) { if (!classAttr->Contains(info->mClasses.ObjectAt(i), @@ -5802,19 +5806,15 @@ DestroyClassNameArray(void* aData) delete info; } -// static -nsresult -nsContentUtils::GetElementsByClassName(nsINode* aRootNode, - const nsAString& aClasses, - nsIDOMNodeList** aReturn) +static void* +AllocClassMatchingInfo(nsINode* aRootNode, + const nsString* aClasses) { - NS_PRECONDITION(aRootNode, "Must have root node"); - nsAttrValue attrValue; - attrValue.ParseAtomArray(aClasses); + attrValue.ParseAtomArray(*aClasses); // nsAttrValue::Equals is sensitive to order, so we'll send an array ClassMatchingInfo* info = new ClassMatchingInfo; - NS_ENSURE_TRUE(info, NS_ERROR_OUT_OF_MEMORY); + NS_ENSURE_TRUE(info, nsnull); if (attrValue.Type() == nsAttrValue::eAtomArray) { info->mClasses.AppendObjects(*(attrValue.GetAtomArrayValue())); @@ -5822,27 +5822,27 @@ nsContentUtils::GetElementsByClassName(nsINode* aRootNode, info->mClasses.AppendObject(attrValue.GetAtomValue()); } - nsBaseContentList* elements; - if (info->mClasses.Count() > 0) { - info->mCaseTreatment = - aRootNode->GetOwnerDoc()->GetCompatibilityMode() == - eCompatibility_NavQuirks ? - eIgnoreCase : eCaseMatters; + info->mCaseTreatment = + aRootNode->GetOwnerDoc()->GetCompatibilityMode() == eCompatibility_NavQuirks ? + eIgnoreCase : eCaseMatters; + return info; +} - elements = - NS_GetFuncStringContentList(aRootNode, MatchClassNames, - DestroyClassNameArray, info, - aClasses).get(); - } else { - delete info; - info = nsnull; - elements = new nsBaseContentList(); - NS_IF_ADDREF(elements); - } - if (!elements) { - delete info; - return NS_ERROR_OUT_OF_MEMORY; - } +// static + +nsresult +nsContentUtils::GetElementsByClassName(nsINode* aRootNode, + const nsAString& aClasses, + nsIDOMNodeList** aReturn) +{ + NS_PRECONDITION(aRootNode, "Must have root node"); + + nsContentList* elements = + NS_GetFuncStringContentList(aRootNode, MatchClassNames, + DestroyClassNameArray, + AllocClassMatchingInfo, + aClasses).get(); + NS_ENSURE_TRUE(elements, NS_ERROR_OUT_OF_MEMORY); // Transfer ownership *aReturn = elements; diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp index 74213633832..eed08fe0f22 100644 --- a/content/html/document/src/nsHTMLDocument.cpp +++ b/content/html/document/src/nsHTMLDocument.cpp @@ -2291,6 +2291,13 @@ nsHTMLDocument::MatchNameAttribute(nsIContent* aContent, PRInt32 aNamespaceID, *elementName, eCaseMatters); } +/* static */ +void* +nsHTMLDocument::UseExistingNameString(nsINode* aRootNode, const nsString* aName) +{ + return const_cast(aName); +} + NS_IMETHODIMP nsHTMLDocument::GetElementsByName(const nsAString& aElementName, nsIDOMNodeList** aReturn) diff --git a/content/html/document/src/nsHTMLDocument.h b/content/html/document/src/nsHTMLDocument.h index 792a404ba7d..953ca58644a 100644 --- a/content/html/document/src/nsHTMLDocument.h +++ b/content/html/document/src/nsHTMLDocument.h @@ -163,13 +163,8 @@ public: nsIContent *GetBody(nsresult *aResult); already_AddRefed GetElementsByName(const nsAString & aName) { - nsString* elementNameData = new nsString(aName); - - return NS_GetFuncStringContentList(this, - MatchNameAttribute, - nsContentUtils::DestroyMatchString, - elementNameData, - *elementNameData); + return NS_GetFuncStringContentList(this, MatchNameAttribute, nsnull, + UseExistingNameString, aName); } // nsIDOMNSHTMLDocument interface @@ -259,6 +254,7 @@ protected: nsIAtom* aAtom, void* aData); static PRBool MatchNameAttribute(nsIContent* aContent, PRInt32 aNamespaceID, nsIAtom* aAtom, void* aData); + static void* UseExistingNameString(nsINode* aRootNode, const nsString* aName); static void DocumentWriteTerminationFunc(nsISupports *aRef);