268932 - Reduce string copying/allocation from CAttributeToken::GetStringValue

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Brian Ryner (not reading)]

CAttributeToken::GetStringValue calls AsString() on its nsScannerSubstring()
which does an unconditional copy of the buffer the first time it's called.  We
can optimize for the case where the string does not span multiple buffers and
return a nsDependentSubstring in that case.  On a test page I loaded
(home.netscape.com on Tp test), Quantify currently shows that malloc() is
responsible for 16.72% of time spent in nsParser::ResumeParse and descendents,
with 7971 calls.  With this patch, that drops to 9.375% with only 6057 calls.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

This works; it's a bit hacky.  Another possibility is moving
nsTDependentSubstring::Rebind up to nsTSubstring and making it know how to free
any existing storage that's been allocated.

Comment 2

[Darin Fisher]

Comment on attachment 165451 [details] [diff] [review]
patch

this looks good to me.	ultimately, i would like to provide a method like
"AssignDependent" on nsSubstring, so that we can make it easier to implement
this kind of thing.

sr=me assuming boris is happy with the patch.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165451 [details] [diff] [review]
patch

>Index: public/nsHTMLTokens.h

>@@ -359,13 +359,13 @@ public:
>   ~CAttributeToken() {}
>   virtual nsresult Consume(PRUnichar aChar,nsScanner& aScanner,PRInt32 aMode);
>   virtual PRInt32 GetTokenType(void);
>-  virtual const nsAString&     GetKey(void); // XXX {return mTextKey;}
>+  virtual const nsSubstring&     GetKey(void); // XXX {return mTextKey;}

Want to make this non-virtual while you're here?

>Index: src/nsScannerString.cpp

>+          mFlattenedRep.~nsString(); // in case we have a buffer currently
>+          new (&mutable_this->mFlattenedRep)
>+            nsDependentSubstring(mStart.mPosition, mEnd.mPosition);

So this makes the following assumptions:

1) The object layout of nsString and nsDependentSubstring is identical
   (including all data and vtables).
2) ~nsString does the right thing when called on the nsDependentSubstring (eg
   doesn't try to free the data).

These are somewhat nontrivial assumptions, since nsString and
nsDependentSubstring aren't even ancestors of each other; they're siblings
(both inheriting from nsSubstring).

Please document those clearly, and do file a followup bug to have a way to do
this that doesn't involve knowing that these classes are somehow "the same"...

r=bzbarsky with that.

Comment 4

[Brian Ryner (not reading)]

checked in. follow-up bug is bug 269054.

Comment 5

[Giacomo Magnini]

This seems to have retaken some of the Tp time (btek) lost during the last few
weeks. Great, thanks!