New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 631931 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

createElement is checking isValidName (of various kinds) many times when custom elements v1 is turned on

Project Member Reported by esprehn@chromium.org, Jul 27 2016

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.

 

Comment 1 by hayato@chromium.org, Jul 27 2016

Cc: kojii@chromium.org
Owner: dominicc@chromium.org
Status: Assigned (was: Untriaged)
dominicc@, could you fix this?
Status: Started (was: Assigned)
Sure.
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.
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by hayato@chromium.org, Oct 12 2016

Components: -Blink>WebComponents Blink>HTML>CustomElements
Cc: esprehn@chromium.org
Status: Fixed (was: Started)
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. 
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