Make custom element name checks faster and fewer

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
Committed: https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790
Cr-Commit-Position: refs/heads/master@{#416959}

[dominicc (has gone to gerrit), 2016-08-29 06:50:05]

Description was changed from

==========
Merge element creation paths to remove a redundant name check.

Element creation is a bit of a twisty maze of passages, all slightly
different. Custom elements added checks for custom element names in
multiple places.

This folds custom element creation for createElement into
HTMLElementFactory::createHTMLElement, avoiding a name check. This
entails passing an optional ExceptionState to the HTMLElementFactory
for the createElement case.

It also weakens some CHECKs that custom elements should be created to
DCHECKs.

BUG=631931,639383
==========

to

==========
Merge element creation paths to remove a redundant name check.

Element creation is a bit of a twisty maze of passages, all slightly
different. Custom elements added checks for custom element names in
multiple places.

This folds custom element creation for createElement into
HTMLElementFactory::createHTMLElement, avoiding a name check. This
entails passing an optional ExceptionState to the HTMLElementFactory
for the createElement case.

It also makes some changes to make things faster, suggested by esprehn
in Issue 639383:

- Weakens some CHECKs that custom elements should be created to DCHECKs.
- Lifts the is8Bit check on custom element tag names to once per string.

BUG=631931,639383
==========

[dominicc (has gone to gerrit), 2016-08-29 06:50:15]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 06:50:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-08-29 06:52:54]

dominicc@chromium.org changed reviewers:
+ esprehn@chromium.org

[dominicc (has gone to gerrit), 2016-08-29 06:52:55]

PTAL

FYI perf tryjob up at https://codereview.chromium.org/2292433002

[dominicc (has gone to gerrit), 2016-08-29 06:54:43]

Description was changed from

==========
Merge element creation paths to remove a redundant name check.

Element creation is a bit of a twisty maze of passages, all slightly
different. Custom elements added checks for custom element names in
multiple places.

This folds custom element creation for createElement into
HTMLElementFactory::createHTMLElement, avoiding a name check. This
entails passing an optional ExceptionState to the HTMLElementFactory
for the createElement case.

It also makes some changes to make things faster, suggested by esprehn
in Issue 639383:

- Weakens some CHECKs that custom elements should be created to DCHECKs.
- Lifts the is8Bit check on custom element tag names to once per string.

BUG=631931,639383
==========

to

==========
Merge element creation paths to remove a redundant name check.

Element creation is a bit of a twisty maze of passages, all slightly
different. Custom elements added checks for custom element names in
multiple places.

This folds custom element creation for createElement into
HTMLElementFactory::createHTMLElement, avoiding a name check. This
entails passing an optional ExceptionState to the HTMLElementFactory
for the createElement case.

It also makes some changes to make things faster, suggested by esprehn
in Issue 631931:

- Weakens some CHECKs that custom elements should be created to DCHECKs.
- Lifts the is8Bit check on custom element tag names to once per string.

BUG=631931,639383
==========

[kouhei (in TOK), 2016-08-29 06:59:55]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2016-08-29 06:59:55]

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/ElementFactory.h.tmpl
(right):

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ElementFactory.h.tmpl:28:
ExceptionState* = 0,
nullptr

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right):

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.h:39: if (!name.length()
|| name[0] < 'a' || name[0] > 'z')
Can we refactor the name[0] check as a platform/text/Character method?

[commit-bot: I haz the power, 2016-08-29 07:59:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 07:59:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[esprehn, 2016-08-30 21:32:32]

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:168:
: createElementSync(document, tagName);
merge them together, there should be a single path with ASSERT_NO_EXCEPTION as
the default.

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h
(right):

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:72:
HTMLElement* createElementSync(Document&, const QualifiedName&,
ExceptionState&);
I think I'd give these different names? Having this method which is the same as
the virtual above is confusing.

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right):

https://codereview.chromium.org/2288653002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.h:72: static
HTMLElement* createCustomElementSync(Document&, const AtomicString& localName,
ExceptionState*);
We normally do ExceptionState& and then default argument = ASSERT_NO_EXCEPTION

[dominicc (has gone to gerrit), 2016-09-01 08:08:22]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 08:08:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-09-01 08:13:31]

PTAL

FYI perf tryjob at https://codereview.chromium.org/2307533002

[dominicc (has gone to gerrit), 2016-09-01 08:15:33]

Description was changed from

==========
Merge element creation paths to remove a redundant name check.

Element creation is a bit of a twisty maze of passages, all slightly
different. Custom elements added checks for custom element names in
multiple places.

This folds custom element creation for createElement into
HTMLElementFactory::createHTMLElement, avoiding a name check. This
entails passing an optional ExceptionState to the HTMLElementFactory
for the createElement case.

It also makes some changes to make things faster, suggested by esprehn
in Issue 631931:

- Weakens some CHECKs that custom elements should be created to DCHECKs.
- Lifts the is8Bit check on custom element tag names to once per string.

BUG=631931,639383
==========

to

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

[commit-bot: I haz the power, 2016-09-01 09:29:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 09:29:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dominicc (has gone to gerrit), 2016-09-02 00:51:41]

PTAL

Perf job result looks OK for createElement.

It claims ID getters regressed 30%! Looked at a profile and it's unsurprising--a
ton of time in id attribute getter callback. Wonder if this is bot weather. I
will rerun the job. https://codereview.chromium.org/2303793003

[esprehn, 2016-09-02 00:53:20]

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right):

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.h:42: // Note: All valid
names start with an ASCII character.
for v1 we could just lookup the element in the registry hash table too since we
don't need to do upgrades for things outside the document?

return registry.contains(name);

why do we care if the element is a custom element but isn't registered?

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.h:48: if
(!Character::isPotentialCustomElementName8BitChar(name[i]))
this is checking is8Bit and isNull() repeatedly inside the loop in the
operator[] accessor of AtomicString.

const LChar* characters = name.characters8();

then use characters[i] in the loop

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.h:54:
U16_NEXT(name.characters16(), i, name.length(), ch);
ditto, you're checking for null repeatedly inside the loop, before the loop do:

const UChar* characters = name.characters16();

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/Character.h (right):

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:86: return 'a' <= ch && ch
<= 'z';
what about upper case? You can just call isASCIILower in the code though, no
need for this method

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:88: static bool
isPotentialCustomElementName8BitChar(UChar ch)
LChar? or template<typename CharType>. Widening the type to UChar makes the code
below slower

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:94: || (0xc0 <= ch && ch !=
0xd7 && ch != 0xf7);
btw I don't know why this is in platform/, can we put all of these methods in
dom/ ? You can do that in another patch though

[esprehn, 2016-09-02 00:55:31]

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right):

https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.h:48: if
(!Character::isPotentialCustomElementName8BitChar(name[i]))
On 2016/09/02 at 00:53:20, esprehn wrote:
> this is checking is8Bit and isNull() repeatedly inside the loop in the
operator[] accessor of AtomicString.
> 
> const LChar* characters = name.characters8();
> 
> then use characters[i] in the loop

This is also doing bounds checks, so you can save 3 branches per character here.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/text/WTFSt...

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/text/Strin...

[dominicc (has gone to gerrit), 2016-09-02 07:43:01]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 07:43:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-09-02 07:47:43]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 07:47:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-09-02 07:48:41]

On 2016/09/02 at 00:55:31, esprehn wrote:
>
https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right):
> 
>
https://codereview.chromium.org/2288653002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElement.h:48: if
(!Character::isPotentialCustomElementName8BitChar(name[i]))
> On 2016/09/02 at 00:53:20, esprehn wrote:
> > this is checking is8Bit and isNull() repeatedly inside the loop in the
operator[] accessor of AtomicString.
> > 
> > const LChar* characters = name.characters8();
> > 
> > then use characters[i] in the loop
> 
> This is also doing bounds checks, so you can save 3 branches per character
here.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/text/WTFSt...
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/text/Strin...

I thought that's what I pay my optimizing compiler writer for? :P

OK, here's one that's more raw.

[esprehn, 2016-09-02 09:22:24]

I think you missed my question about just looking up names in the registry. I
understand the need for this check when doing defineElement, but not when doing
createElement

https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/Character.h (right):

https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:86: return 'a' <= ch && ch
<= 'z';
Can we just use isASCIILower :)

https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:88: static bool
isPotentialCustomElementName8BitChar(UChar ch)
LChar since this is 8bit

https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:91: return ('a' <= ch && ch
<= 'z')
isASCIILower || isASCIIDigit || ...

[commit-bot: I haz the power, 2016-09-02 09:28:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 09:28:05]

Dry run: This issue passed the CQ dry run.

[dominicc (has gone to gerrit), 2016-09-03 22:39:10]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-03 22:39:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-09-03 22:52:01]

On 2016/09/02 at 09:22:24, esprehn wrote:
> I think you missed my question about just looking up names in the registry. I
understand the need for this check when doing defineElement, but not when doing
createElement

Sorry, yes, I did miss that.

Custom elements which don't have a definition are still processed differently to
other elements. Compare

document.createElement('foo')
document.createElement('x-foo')

for example: foo is HTMLUnknownElement; x-foo is HTMLElement. foo is not an
upgrade candidate; x-foo is an upgrade candidate.

You had another question/point about putting these in CustomElement instead of
Character. Koji wrote it so he will know, but I guess it was expedient because
all of the lookup table machinery is built for Character. And Kouhei asked to
then keep the related things together, see comment 8.

You had another question about uppercase letters or lowercasing the name. This
is simply a matter of what is allowed by the spec--only lowercase a-z are
allowed as initial letters.

>
https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/text/Character.h (right):
> 
>
https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/text/Character.h:86: return 'a' <= ch && ch
<= 'z';
> Can we just use isASCIILower :)

OK, done.

>
https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/text/Character.h:88: static bool
isPotentialCustomElementName8BitChar(UChar ch)
> LChar since this is 8bit

Done!

>
https://codereview.chromium.org/2288653002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/text/Character.h:91: return ('a' <= ch &&
ch <= 'z')
> isASCIILower || isASCIIDigit || ...

Done!

[commit-bot: I haz the power, 2016-09-04 00:12:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-04 00:12:59]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-09-07 03:54:05]

lgtm, though I'd really like us to file a spec bug and just not allow stuff
outside ASCII in v1. It makes all of this stuff much slower permitting emoji and
random latin1 stuff in there. :/

https://codereview.chromium.org/2288653002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/Character.h (right):

https://codereview.chromium.org/2288653002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/Character.h:89: || ch == '-' || ch ==
'.' || ch == '_' || ch == 0xb7
what char is 0xb7?

https://codereview.chromium.org/2288653002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/Character.h:90: || (0xc0 <= ch && ch !=
0xd7 && ch != 0xf7);
What latin1 chars are these?

[dominicc (has gone to gerrit), 2016-09-07 15:06:03]

On Tue, Sep 6, 2016 at 8:54 PM, <esprehn@chromium.org> wrote:

> lgtm, though I'd really like us to file a spec bug and just not allow stuff
> outside ASCII in v1. It makes all of this stuff much slower permitting
> emoji and
> random latin1 stuff in there. :/
>

I don't think these name checks are that expensive, but I've filed a bug
just the same; maybe you could chime in there:

https://github.com/whatwg/html/issues/1754

It makes sense to me to be liberal in what characters are allowed; HTML
allows a wide range of characters for element names already and this seems
consistent with that.


>
> https://codereview.chromium.org/2288653002/diff/100001/
> third_party/WebKit/Source/platform/text/Character.h
> File third_party/WebKit/Source/platform/text/Character.h (right):
>
> https://codereview.chromium.org/2288653002/diff/100001/
> third_party/WebKit/Source/platform/text/Character.h#newcode89
> third_party/WebKit/Source/platform/text/Character.h:89: || ch == '-' ||
> ch == '.' || ch == '_' || ch == 0xb7
> what char is 0xb7?
>

Georgian middle dot; I think that's something like punctuation.


> https://codereview.chromium.org/2288653002/diff/100001/
> third_party/WebKit/Source/platform/text/Character.h#newcode90
> third_party/WebKit/Source/platform/text/Character.h:90: || (0xc0 <= ch
> && ch != 0xd7 && ch != 0xf7);
> What latin1 chars are these?
>
> https://codereview.chromium.org/2288653002/
>

Multiplication and division signs.

FWIW these character ranges have history, from XML via HTML or something.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-09-07 15:06:07]

On Tue, Sep 6, 2016 at 8:54 PM, <esprehn@chromium.org> wrote:

> lgtm, though I'd really like us to file a spec bug and just not allow stuff
> outside ASCII in v1. It makes all of this stuff much slower permitting
> emoji and
> random latin1 stuff in there. :/
>

I don't think these name checks are that expensive, but I've filed a bug
just the same; maybe you could chime in there:

https://github.com/whatwg/html/issues/1754

It makes sense to me to be liberal in what characters are allowed; HTML
allows a wide range of characters for element names already and this seems
consistent with that.


>
> https://codereview.chromium.org/2288653002/diff/100001/
> third_party/WebKit/Source/platform/text/Character.h
> File third_party/WebKit/Source/platform/text/Character.h (right):
>
> https://codereview.chromium.org/2288653002/diff/100001/
> third_party/WebKit/Source/platform/text/Character.h#newcode89
> third_party/WebKit/Source/platform/text/Character.h:89: || ch == '-' ||
> ch == '.' || ch == '_' || ch == 0xb7
> what char is 0xb7?
>

Georgian middle dot; I think that's something like punctuation.


> https://codereview.chromium.org/2288653002/diff/100001/
> third_party/WebKit/Source/platform/text/Character.h#newcode90
> third_party/WebKit/Source/platform/text/Character.h:90: || (0xc0 <= ch
> && ch != 0xd7 && ch != 0xf7);
> What latin1 chars are these?
>
> https://codereview.chromium.org/2288653002/
>

Multiplication and division signs.

FWIW these character ranges have history, from XML via HTML or something.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-09-07 15:06:30]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2016-09-07 15:06:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 16:37:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-07 16:39:16]

Description was changed from

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

to

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

Committed: https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790
Cr-Commit-Position: refs/heads/master@{#416959}
==========

[commit-bot: I haz the power, 2016-09-07 16:39:17]

Patchset 6 (id:??) landed as
https://crrev.com/a67c4b53027ee270f17241d7a594ac10aef33790
Cr-Commit-Position: refs/heads/master@{#416959}