Bug 99394

In the RoboHornet svgresize.html benchmark, I'm seeing ~200ms below QualifiedName instantiation and hash lookups on my MBP. I have a patch to cut this in half.

Created attachment 168815 [details] Proposed patch

There is some debate as to if we should even be using QualifiedNames here. anyQName() is this odd hack that we seem to use to be the "null" QualifiedName, and it's basically only used for SVG code: http://code.google.com/p/chromium/source/search?q=anyQName&origq=anyQName&btnG=Search+Trunk I think this patch is totally reasonable, and I can see it being a huge speed win. I'm just not sure if the concept of a null QualifieName (or using QualfiedName in a hash) even makes sense as we currently do it.

This patch makes me wonder if we shouldn't just ditch anyQName in prefernce for this nullQName anyway. :)

Love this patch. Nice work!

Comment on attachment 168815 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=168815&action=review > Source/WebCore/dom/QualifiedName.h:134 > + QualifiedNameComponents c = { name->m_prefix.impl(), name->m_localName.impl(), name->m_namespace.impl() }; > + name->m_existingHash = hashComponents(c); I think it’d be nice to have this part be out of line to keep the inlined code tighter. Worth testing to see if it makes things faster or slower. If it’s not slower, we should do it. Also, I’d name the local variable something other than c. > Source/WebCore/svg/SVGElement.h:164 > + QualifiedNameComponents c = { nullAtom.impl(), key.localName().impl(), key.namespaceURI().impl() }; > + return hashComponents(c); It’s be nice to name this something other than c. I think we could cache this no-prefix form of the qualified name too; just add another field to QualifiedName for that.

Comment on attachment 168815 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=168815&action=review > Source/WebCore/ChangeLog:3 > + Clean up QualifiedName-as-hash-key scenario. I would call this “optimize” not “clean up”.

anyQName is super mysterious to me. From the name and the "*" strings in it, I would expect it to have some sort of wildcard semantic, but it doesn't really. I wonder if Hyatt remembers what it's for?

Oddly enough anyQName was added by hyatt! http://trac.webkit.org/changeset/9824 Just hijacked by SVG for null purposes. :(

(In reply to comment #9) > Oddly enough anyQName was added by hyatt! http://trac.webkit.org/changeset/9824 Just hijacked by SVG for null purposes. :( Indeed, I remembered that he added it because I reviewed that patch. But I do not remember the original purpose.

Committed <http://trac.webkit.org/changeset/131993> (Darn, forgot to change the bug title in the ChangeLog.)

(In reply to comment #11) > Committed <http://trac.webkit.org/changeset/131993> > (Darn, forgot to change the bug title in the ChangeLog.) This progression is visible in various webkit-perf graphs (bigger is better): http://webkit-perf.appspot.com/graph.html#tests=[[8195914,2001,173262]]&sel=1350758073445.8484,1350797740907.5178,0.9000000000000004,6&displayrange=7&datatype=running Nice work!