createElement is checking isValidName (of various kinds) many times when custom elements v1 is turned on |
||||
Issue description
Note: CustomElement::shouldCreateCustomElement does isValidName.
Document::createElement (the script one) does:
if (!isValidName(name)) { <-- First time, scan the whole name.
exceptionState.throwDOMException(InvalidCharacterError, "The tag name provided ('" + name + "') is not a valid name.");
return nullptr;
}
if (isXHTMLDocument() || isHTMLDocument()) {
if (CustomElement::shouldCreateCustomElement(*this, name))
return CustomElement::createCustomElementSync(*this, name, exceptionState);
return HTMLElementFactory::createHTMLElement(convertLocalName(name), *this, 0, CreatedByCreateElement);
}
HTMLElementFactory::createHTMLElement does:
if (CustomElement::shouldCreateCustomElement(document, localName)) {
QualifiedName tagName(nullAtom, localName, HTMLNames::xhtmlNamespaceURI);
if (flags & AsynchronousCustomElements)
return CustomElement::createCustomElementAsync(document, tagName);
return CustomElement::createCustomElementSync(document, tagName);
}
CustomElement::shouldCreateCustomElement does:
return RuntimeEnabledFeatures::customElementsV1Enabled()
&& document.frame() && isValidName(localName); <-- Again, scan, possible hash lookup.
CustomElement::createCustomElementSync (and Async) does:
CHECK(shouldCreateCustomElement(document, tagName)); <-- Again.
- If you have an unknown tag name like "foo", we scan the tag name 3+ times, one inside a CHECK is just wasted work full of branching.
- We shouldn't be using doing a CHECK that scans the entire tag name and does a hash lookup.
- Valid custom element names get scanned 3 times.
- Valid platform names get scanned 2 times.
Note that CustomElement::isValidName is also doing very expensive things, like checking is8Bit and name for null for every single character and inflating to UChar32 when nearly all custom element names are ascii.
,
Jul 28 2016
Sure.
,
Aug 29 2016
Thanks for filing this. Patch up at https://codereview.chromium.org/2288653002 PTAL. Note sheriffs filed Issue 639383 which is essentially the same. Should I de-dup? I'd like to keep their issue around for tracking, but I don't want to forward-dedup your great bug. So for now I just kept them both open.
,
Sep 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a67c4b53027ee270f17241d7a594ac10aef33790 commit a67c4b53027ee270f17241d7a594ac10aef33790 Author: dominicc <dominicc@chromium.org> Date: Wed Sep 07 16:36:58 2016 Make custom element name checks faster and fewer This makes some changes to make things faster, suggested by esprehn in Issue 631931 and discussion: - Weakens some CHECKs that custom elements should be created to DCHECKs. - Specializes 8-bit strings. - Does a quick reject for names without hyphens. - Does one fewer name checks in the createElement case. BUG= 631931 , 639383 Review-Url: https://codereview.chromium.org/2288653002 Cr-Commit-Position: refs/heads/master@{#416959} [modify] https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790/third_party/WebKit/Source/build/scripts/templates/ElementFactory.cpp.tmpl [modify] https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790/third_party/WebKit/Source/core/dom/custom/CustomElement.cpp [modify] https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790/third_party/WebKit/Source/core/dom/custom/CustomElement.h [modify] https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790/third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp [modify] https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790/third_party/WebKit/Source/platform/text/Character.h
,
Oct 12 2016
,
Oct 27 2016
The most egregious performance regression was fixed, see Issue 639383 . We could implement a special valid name check that also checks custom element name validity and passes that result on in the custom element case to combine some checks, although it would complicate the createElement('div') case with the speculative custom element name check. I'm going to close this for now, but esprehn if you feel we should unify the name checks further, please reopen and I'll get right on it.
,
Oct 27 2016
If we proceed with https://bugs.chromium.org/p/chromium/issues/detail?id=648179 and end up removing some of createElement's restrictions, we can also simplify the checks used for custom elements, since those were by design the intersection of allowed names for the parser and for createElement. |
||||
►
Sign in to add a comment |
||||
Comment 1 by hayato@chromium.org
, Jul 27 2016Owner: dominicc@chromium.org
Status: Assigned (was: Untriaged)