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.

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?

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.

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.

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).

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?

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

> 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.

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.

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.

-> 1.2

See also bug 88411.

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?

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.

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

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.