nsStringKey is too expensive because of nsAString

RESOLVED INVALID

Status

()

Core
DOM
RESOLVED INVALID
16 years ago
15 years ago

People

(Reporter: Daniel Bratell, Assigned: jag (Peter Annema))

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla1.2alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

(Reporter)

Description

16 years ago
As much as 10-20% of simple DOM calls are spent in ToNewUnicode inside the
nsStringKey constructor. 

The constructor is called from nsScriptNameSpaceManager::LookupName with a
nsAReadableString (typedeffed const nsAString) and from
nsHTMLDocument::ResolveName with the same type of argument. This causes the
nsStringKey::nsStringKey(const nsAString& str) construct to be called which
allocates a completely new string with ToNewUnicode.

It's extremely unlikely that these strings are something else than flat strings
(maybe not null terminated but still flat) so it shouldn't be necessary to copy
the string. In the actual cases I've looked at the nsASingleFragment.Length()
function gets called so the strings are some type of nsASingleFragment.

I seem to remember an nsPromiseFlatString function. Maybe that could be used
some  way?

I'm donating this bug to jag since I think you know the string libraries quite
well. Feel free reassign it.
(Reporter)

Updated

16 years ago
Blocks: 118933
Keywords: perf
OS: Windows 2000 → All
Hardware: PC → All
The problem is that the string key _could_ use PromiseFlatString but it would
need to keep the flat string around for the life of the key, otherwise the
buffer it's using would get deallocated.  This would get pretty messy.

However, there is a |nsStringKey::nsStringKey(const nsAFlatString& str)| version
of the constructor... Should the calling code just use PromiseFlatString()
itself? I'll whip up a patch that does that.  Daniel, would you test the perf
impact?
Created attachment 64736 [details] [diff] [review]
First cut at not doing 

Use flat strings in those two cases.  Also, fix an unsafe key creation in
nsScriptNameSpaceManager::FillHashWithDOMInterfaces.  Daniel, would you test
this one out?

Looking at the code in nsScriptNameSpaceManager, LookupName is the only time we
have a Unicode string and create a key.  In all other cases, we have CStrings
that we convert to Unicode before creating keys.  Not sure whether we can win
anything by going to nsCStringKey and maybe changing LookupName to take an
nsACString argument....
The two callers of LookupName are StubConstructor() and
nsWindowSH::GlobalResolve() both in nsDOMClassInfo.cpp.

StubConstructor() has a char* that it converts to Unicode to call LookupName. 
GlobalResolve() seems to start with a unicode string.  At a guess, this last is
responsible for most of the LookupName calls and LookupName is doing most of the
hash access.  So going to CStringKeys is probably a bad idea.

Comment 4

16 years ago
Note that lookupName is called _very_ often because GlobalResolve() is called
_very_ often. I don't know if the nsStringKey problem mentionned in this bug
also arises when there is no match found, but if yes, then it's potentially bad,
given the amount of calls.
(Reporter)

Comment 5

16 years ago
Comment on attachment 64736 [details] [diff] [review]
First cut at not doing 

Seems to be good changes!

I did no wall time measurements but Quantify says that the getElementById test
is 50-150ms (~10%) faster with the change. Disregarding the security which is a
known performance problem, this seems to make getElementById more than 20%
faster.

One more change though, nsHTMLDocument::GetElementById also needs the
nsPromiseFlatString. I guess it's needed in many more places really, but in the
testcase I ran, only nsHTMLDocument::GetElementById showed up.
Hmm.  We could go around adding this all over (I had noticed the use in
GetElementById as well, but wanted to see what sort of impact we would have
here). I think we should make some sort of arch decision here.... Should the
nsAString constructor just be removed, forcing callers to flatten their string
keys?  Should we try to call PromiseFlatString inside the constructor after all
and just store the alias somewhere for the life of the key?  Should we just add
a big "This is slow" comment to the nsAString constructor and change all the
perf-conscious callers to use flat strings?

Jag, is there a way to safely cast nsAStrings to nsAFlatStrings in cases when
they are actually flat?  Is there even a way to tell that they are flat?
In stead of adding PromiseFlatString()'s (which are actually fairly expensive)
in the DOM code, we should change the signarure of LookupName() to take const
nsAFlatString& since all strings that are passed to that method are flat. Simple
and faster than using PromiseFlatString(). Patch coming up (including bz's
NS_ConvertASCIItoUCS2() useage fix).
Created attachment 64779 [details] [diff] [review]
Change signature of nsScriptNameSpaceManager::LookupName()...
That's certainly the simplest solution. :)  Could we whack
nsHTMLDocument::ResolveName in a similar way?  Or is that too close to the DOM
interfaces?

What about nsHTMLDocument::GetElementById?

As for making nsHTMLDocument::GetElementById() use PrmoiseFlatString() to avoid
having nsStringKey() allocate a string copy, it turns out from my testing that
doing that doesn't buy us any thing. The reason being that PromiseFlatString()
ends up calling GetBufferHandle() on a XPCReadableJSStringWrapper(), which then
must allocate the buffer handle, so we still end up doing a malloc, and we get
the overhead of PromiseFlatString(), which seems to be higher than the
ToNewUnicode() we get when using nsStringKey(const nsAString&).

I was thinking of adding some kind of buffer handle recycling shceme to
XPCReadableJSStringWrapper(), but that isn't possible since the buffer handle
baseclass (nsBufferHandle) has no virtual methods, so I can't override the
destructor nor the ReleaseReference() method in nsSharedBufferHandle. Jag, this
sucks. Want a separate bug on the lack of virtual methods in the buffer handle code?
(Reporter)

Comment 11

16 years ago
Yes, that's better in the LookupName case but we should still find a solution
for all the other functions. (I'm not sure about GetElementByID - it seems to
have some strange form of string class I've never seen before that allocates a
buffer on Length() call).

nsPromiseFlatString might be expensive in a way but ToNewUnicode seems to a 100
times more expensive when both are used on already flat strings so I rather do
lots of nsPromiseFlatString than one ToNewUnicode. 

Adding nsPromiseFlatString to all callers deoesn't sound like a good solution
though. It would increase code size and it would be easy to forget when
modifying/adding new code. The only way to avoid that would be to remove the
nsStringKey(nsAString&) constructor but I don't think we want that either. 
Yeah, nsIHTMLDocument::ResolveName() could be hacked in a similar way.

nsStringKey is really lame in other ways too, especially when used on strings
that come from the JS engine. They're PRUnichar* based, thus embedded '\0'
character's will cause them to not do the right thing, i.e. in JS the two
following expressions will yield true:

  window['Node'] === window['Node\0foo']
  document.getElementById('foo') === document.getElementById('foo\0bar')

Not likely, but still wrong.
Comment on attachment 64779 [details] [diff] [review]
Change signature of nsScriptNameSpaceManager::LookupName()...

r=bzbarsky
Attachment #64779 - Flags: review+
Attachment #64736 - Attachment is obsolete: true
> nsPromiseFlatString might be expensive in a way but ToNewUnicode seems to a 100
> times more expensive when both are used on already flat strings so I rather do
> lots of nsPromiseFlatString than one ToNewUnicode. 

I've seen one nsPromiseFlatString() be more expensive than one ToNewUnicode() in
many cases, at least when used with the string class XPCReadableJSStringWrapper.
(Reporter)

Comment 15

16 years ago
Ah, that explains some things. 

But the fix in LookupNames and something similar in nsHTMLDocument::ResolveName
would be quite good. Those are the functions that are called alot. We can leave
this bug open if we want to work more on it after that first checkin.
Comment on attachment 64779 [details] [diff] [review]
Change signature of nsScriptNameSpaceManager::LookupName()...

Oh.  And add a comment explaining why we use
nsAFlatString instead of nsAString, lest some kind
soul change it back?
Consider a comment added.
FYI, the dom code touched in the attached patches here is further optimized in
bug 118933, the changes in that bug completely eliminates the nsStringKey useage
from that code.
(Assignee)

Comment 19

16 years ago
jst, does any work still need to be done for this bug?
Well, nsStringKey didn't change, and is still expensive to use with nsAString
references, but I'm not sure there's anything we can do about that other than
not use them.
(Assignee)

Comment 21

16 years ago
-> 1.2
Target Milestone: --- → mozilla1.2
See also bug 88411.
(Assignee)

Comment 23

15 years ago
Comment on attachment 64779 [details] [diff] [review]
Change signature of nsScriptNameSpaceManager::LookupName()...

sr=jag

I'm guessing some callers might need to be fixed for this?
Attachment #64779 - Flags: superreview+
(Assignee)

Comment 24

15 years ago
Comment on attachment 64779 [details] [diff] [review]
Change signature of nsScriptNameSpaceManager::LookupName()...

Then again, ::LookupName doesn't use nsStringKey any longer, so that patch
isn't needed.
Attachment #64779 - Attachment is obsolete: true
Attachment #64779 - Flags: superreview+ → superreview-

Comment 25

15 years ago
Can this bug be WONTFIXed? If we still have performance issues, I would be happy
to convert users to the nsTHashtable variants...

Comment 26

15 years ago
where are we with this? do we need a new patch?
I'd say we're done with this one, if someone disagrees, please reopen and
explain what's left to do here. Marking INVALID since it doesn't really apply
any more.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.