1390342 - The search for attributes under SingleLineTextInputTypeBase::HasPatternMismatch() shows up in profiles

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[(no longer active)]

See this code:

https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/html/input/SingleLineTextInputTypes.cpp#76

This gets called from HTMLInputElement::UpdateAllValidityStates() which gets called from HTMLInputElement::OnValueChanged().  It would be nice if we can avoid this search in the most common cases by remembering whether we have a pattern attribute in a flag before searching for it.

Jessica, are you interested in this bug by any chance?  This is similar to the other bugs in this area which you have fixed recently.  :-)

Comment 1

[Jessica Jong [:jessica] (inactive)]

Sure, I can help on this.

So, to be sure, we should introduce a flag, something like, NS_EVENT_STATE_HAS_PATTERN_ATTRIBUTE, and only cache whether the element has the attribute set, but not the pattern string, right?

Comment 2

[(no longer active)]

Yes, basically if the attribute is set we'd fall back to what we currently do in HasPatternMismatch() (IOW walk over the array of attributes to search for the pattern attribute and then match the pattern string.)  The idea behind the optimization is that most input elements on the web will not have a pattern attribute set, so with that optimization for most elements we won't do any work (besides checking the flag of course.)

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Introduce HTML_INPUT_HAS_PATTERN_ATTR boolean flag.

I stole an unused flag bit from Element since we have run out of space. :(

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Skip pattern matching in HasPatternMismatch() if @pattern is not set.

Comment 5

[(no longer active)]

Comment on attachment 8897662 [details] [diff] [review]
Part 1: Introduce HTML_INPUT_HAS_PATTERN_ATTR boolean flag.

Review of attachment 8897662 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, but I really think we shouldn't use an Element flag for this.  I realize now I should have probably mentioned this from the beginning.  My apologies for the extra round trip.

::: dom/html/nsGenericHTMLElement.h
@@ +1019,5 @@
>  };
>  
>  // NOTE: I don't think it's possible to have both ADDED_TO_FORM and
>  // MAYBE_ORPHAN_FORM_ELEMENT set at the same time, so if it becomes an issue we
>  // can probably merge them into the same bit.  --bz

Hmm, I'm really torn on what the right thing to do here is.

I think consuming one flag from Element for something that only one node type uses is quite terrible, these flags are quite valuable and as you noted we are running out of them.  :-(  There is the idea of merging these two here, but it's a lot of more work, and I kind of feel bad to make you do all of that work as a prerequisite for this bug.

There is another possible alternative which is using the existing bitfield in HTMLInputElement, what do you think about that? <https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/dom/html/HTMLInputElement.h#1669>  That's certainly the easiest approach (but freeing a form flag is certainly really valuable work, but please note that I haven't spent enough time to double check whether it's actually possible in this case or not.

Comment 6

[(no longer active)]

Comment on attachment 8897664 [details] [diff] [review]
Part 2: Skip pattern matching in HasPatternMismatch() if @pattern is not set.

Review of attachment 8897664 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is correct but the syntax of setting and resetting the flag may change based on how you decide to change part 1, but r=me on this part anyway.  Thanks a lot!

::: dom/html/HTMLInputElement.cpp
@@ +1457,5 @@
>        UpdateTooLongValidityState();
>      } else if (aName == nsGkAtoms::minlength) {
>        UpdateTooShortValidityState();
> +    } else if (aName == nsGkAtoms::pattern) {
> +      if (IsSingleLineTextControl(false)) {

Nit: Even though you won't use the flag for other types of input types, I think setting and resetting it is harmless.  I'd probably do that, and add a comment saying that this is intentional, in order to save having to check the type here.  Up to you either way!

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #5)
> Comment on attachment 8897662 [details] [diff] [review]
> Part 1: Introduce HTML_INPUT_HAS_PATTERN_ATTR boolean flag.
> 
> Review of attachment 8897662 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, but I really think we shouldn't use an Element flag for this.  I
> realize now I should have probably mentioned this from the beginning.  My
> apologies for the extra round trip.

No problem at all, I didn't mention using a Element flag in the beginning. :)

> 
> ::: dom/html/nsGenericHTMLElement.h
> @@ +1019,5 @@
> >  };
> >  
> >  // NOTE: I don't think it's possible to have both ADDED_TO_FORM and
> >  // MAYBE_ORPHAN_FORM_ELEMENT set at the same time, so if it becomes an issue we
> >  // can probably merge them into the same bit.  --bz
> 
> Hmm, I'm really torn on what the right thing to do here is.
> 
> I think consuming one flag from Element for something that only one node
> type uses is quite terrible, these flags are quite valuable and as you noted
> we are running out of them.  :-(  There is the idea of merging these two
> here, but it's a lot of more work, and I kind of feel bad to make you do all
> of that work as a prerequisite for this bug.
> 
> There is another possible alternative which is using the existing bitfield
> in HTMLInputElement, what do you think about that?
> <https://searchfox.org/mozilla-central/rev/
> 13148faaa91a1c823a7d68563d9995480e714979/dom/html/HTMLInputElement.h#1669> 
> That's certainly the easiest approach (but freeing a form flag is certainly
> really valuable work, but please note that I haven't spent enough time to
> double check whether it's actually possible in this case or not.

Yeah, I think for this bug we can just use the existing bitfield in HTMLInputElement, and maybe file a separate bug to investigate how (or is it possible) to merge those two form flags.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Skip pattern matching in HasPatternMismatch() if @pattern is not set, v2.

Use the existing bitfield in HTMLInputElement to cache if the input element has the pattern attribute set.
Ehsan, since tha patch has become simpler, I merged it into one patch. Could take a look again?

Comment 9

[(no longer active)]

Comment on attachment 8898163 [details] [diff] [review]
Skip pattern matching in HasPatternMismatch() if @pattern is not set, v2.

Review of attachment 8898163 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLInputElement.h
@@ +338,5 @@
>      MOZ_ASSERT(mType == NS_FORM_INPUT_NUMBER);
>      return mSelectionProperties;
>    }
>  
> +  bool HasPatternAttribute() const {

Nit: open brace goes on its own line!

(One day clang-format will free us from adjusting whitespace during review!)

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: Skip pattern matching in HasPatternMismatch() if @pattern is not set, v3.

Thanks ehsan.

Comment 11

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2226d9e32abd12f6dd6ed61d0adcaab0ee369d33&group_state=expanded

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5af0df6016
Skip pattern matching in HasPatternMismatch() if @pattern is not set. r=ehsan

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c5af0df6016