Bug 138524 – Lazily create HTMLInputElement's inputType and shadow subtree

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Lazily create HTMLInputElement's inputType and shadow subtree when created by the parser to avoid having to:
- Create a text inputType and its shadow subtree
- then when the type attribute is set (to something else than 'text'), destroy that inputType and shadow subtree
- Create an inputType of the right type and create its shadow subtree

When created by the parser, the attributes will be set right after constructing the HTMLInputElement. We can thus delay the inputType / shadow subtree until the attributes are set by the parser to make sure we construct those for the right |type| directly.

[Chris Dumez]

Created attachment 241216 [details]
Patch

[Darin Adler]

Comment on attachment 241216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241216&action=review

Should we make this even more lazy, and call ensureInputType when we use m_inputType instead? Maybe some input elements could live a long time without allocating m_inputType, if we make enough functions know how to handle null.

> Source/WebCore/ChangeLog:10
> +        the constucted input. With the previous implementation, this was a bit

Typo: constucted

> Source/WebCore/dom/Element.cpp:1237
> +        if (document().sharedObjectPool())

Strange that some documents would not have this pool. Is that an important optimization?

> Source/WebCore/html/HTMLInputElement.cpp:446
> +    bool didStoreValue = true;
> +    bool neededSuspensionCallback = false;
> +    bool didRespectHeightAndWidth = true;
> +    bool hasUserAgentShadowRoot = false;

Would be good to give a rationale for these. I presume they are the values that would be correct for type text. Note that it is not obvious why these are correct, which is why I’m asking for a comment.

> Source/WebCore/html/HTMLInputElement.cpp:639
> +    if (m_parsingInProgress) {
> +        // Attributes have now been set by the parser and we should now make sure we have an
> +        // input type and a user-agent shadow root.
> +        ensureInputType();
> +    }

I don’t understand why we have both this and the call in finishParsingAttributes below. Is this correct?

[Chris Dumez]

Comment on attachment 241216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241216&action=review

>> Source/WebCore/dom/Element.cpp:1237
>> +        if (document().sharedObjectPool())
> 
> Strange that some documents would not have this pool. Is that an important optimization?

I am not very familiar with this optimization. I merely changed the indentation here.
From what I can see, the Document is supposed to have a sharedObjectPool() during parsing and a little bit after (it is being cleared via a timer).

>> Source/WebCore/html/HTMLInputElement.cpp:446
>> +    bool hasUserAgentShadowRoot = false;
> 
> Would be good to give a rationale for these. I presume they are the values that would be correct for type text. Note that it is not obvious why these are correct, which is why I’m asking for a comment.

Yes, those are the values to type text. I will see if I can make this clearer. Worse case scenario, I'll add a comment to explain.

>> Source/WebCore/html/HTMLInputElement.cpp:639
>> +    }
> 
> I don’t understand why we have both this and the call in finishParsingAttributes below. Is this correct?

This is to initialize m_inputType as soon as we can (i.e. as soon as parseAttribute() gets called for the first attribute). This is needed because the code below in this method relies on m_inputType being initialized.
The call to ensureInputType() in finishParsingAttributes() is a safety net in the element has no attribute. In such case, parseAttribute() will no be called and thus m_inputType won't be initialized. This makes sure m_inputType is initialized by the end of parsing.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 241216 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241216&action=review
> 
> Should we make this even more lazy, and call ensureInputType when we use
> m_inputType instead? Maybe some input elements could live a long time
> without allocating m_inputType, if we make enough functions know how to
> handle null.

I'll check if we can relatively cleanly do that. One issue is that it is not only m_inputType that it lazy initialized but also the user-agent shadow root, and the shadow subtree.

[Chris Dumez]

Created attachment 241293 [details]
Patch

[Chris Dumez]

Comment on attachment 241293 [details]
Patch

Requesting review again as I changed the structured a little bit. Trying to reuse updateType() for m_inputType lazy initialization did not look as great as I hoped (because not enough code is shared between initial initialization and actual type update, and also because of variables initialization for the initial initialization case). As a result, ensureInputType() now takes care of initialization m_inputType and no longer calls updateType(). This simplifies the code a lot.

Some code was also moved to a new runPostTypeUpdateTasks() method to enable code sharing between ensureInputType() and updateType().

[Ryosuke Niwa]

Comment on attachment 241293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241293&action=review

> Source/WebCore/dom/Element.cpp:1247
> +    finishParsingAttributes();

I'm a bit worried about the cost of calling this virtual function on every element.
Did you run Dromaeo, etc... to make sure this isn't a regression on other tests?

> Source/WebCore/html/HTMLInputElement.cpp:136
> +    // The user-agent shadowRoot is lazily created when created by the parser.
> +    if (!createdByParser)

Why don't we create a local boolean named shouldCreateShadowRootLazily = createdByParser instead of adding a comment?

> Source/WebCore/html/HTMLInputElement.cpp:634
> +        // Attributes have now been set by the parser and we should now make sure we have an
> +        // input type and a user-agent shadow root.
> +        ensureInputType();

It's not clear to me why we'd have to call ensureInputType right away even during parsing...
I'd replace this "what" comment by explaining that "why" if I were you.

> Source/WebCore/html/HTMLInputElement.cpp:744
> +void HTMLInputElement::finishParsingAttributes()

I would call this function parserDidFinishParsingAttributes instead since "parseAttribute" can be called via DOM API as well.

> Source/WebCore/html/HTMLInputElement.cpp:748
> +    // Attributes have now been set by the parser and we should now make sure we have
> +    // an input type in case parseAttribute() wasn't called (i.e. there was no
> +    // attribute).

I would prefer asserting that the UA shadow root is there when there are attributes instead of having a comment like this.

[Chris Dumez]

Comment on attachment 241293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241293&action=review

>> Source/WebCore/dom/Element.cpp:1247
>> +    finishParsingAttributes();
> 
> I'm a bit worried about the cost of calling this virtual function on every element.
> Did you run Dromaeo, etc... to make sure this isn't a regression on other tests?

I will check Dromaeo.

>> Source/WebCore/html/HTMLInputElement.cpp:136
>> +    if (!createdByParser)
> 
> Why don't we create a local boolean named shouldCreateShadowRootLazily = createdByParser instead of adding a comment?

Hmm, I have a slight preference for the comment in this case, but OK.

>> Source/WebCore/html/HTMLInputElement.cpp:634
>> +        ensureInputType();
> 
> It's not clear to me why we'd have to call ensureInputType right away even during parsing...
> I'd replace this "what" comment by explaining that "why" if I were you.

A lot of the code below in this function relies on m_inputType being initialized, either directly (by dereferencing m_inputType), or indirectly by calling removeFromRadioButtonGroup(), updateType(), needsSuspensionCallback(), ...
Also note that, by the time parseAttribute() gets called during parsing, all attributes have already been set on the Element by the parser and there is no point in delaying m_inputType initialization any longer.

I will try and clarify the comment.

>> Source/WebCore/html/HTMLInputElement.cpp:744
>> +void HTMLInputElement::finishParsingAttributes()
> 
> I would call this function parserDidFinishParsingAttributes instead since "parseAttribute" can be called via DOM API as well.

I tried to be consistent with finishParsingChildren() but OK, I see your point.

>> Source/WebCore/html/HTMLInputElement.cpp:748
>> +    // attribute).
> 
> I would prefer asserting that the UA shadow root is there when there are attributes instead of having a comment like this.

Ok, I will replace with ASSERT(m_inputType || !hasAttributes());

[Chris Dumez]

Comment on attachment 241293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241293&action=review

>>> Source/WebCore/dom/Element.cpp:1247
>>> +    finishParsingAttributes();
>> 
>> I'm a bit worried about the cost of calling this virtual function on every element.
>> Did you run Dromaeo, etc... to make sure this isn't a regression on other tests?
> 
> I will check Dromaeo.

I don't see any impact on Dromaeo:
http://dromaeo.com/?id=230051,230054

[Chris Dumez]

Comment on attachment 241293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241293&action=review

>>>> Source/WebCore/dom/Element.cpp:1247
>>>> +    finishParsingAttributes();
>>> 
>>> I'm a bit worried about the cost of calling this virtual function on every element.
>>> Did you run Dromaeo, etc... to make sure this isn't a regression on other tests?
>> 
>> I will check Dromaeo.
> 
> I don't see any impact on Dromaeo:
> http://dromaeo.com/?id=230051,230054

I don't see any regression locally on PerformanceTests/Parser tests either.

[Chris Dumez]

Created attachment 241323 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241323 [details]
Patch

Clearing flags on attachment: 241323

Committed r175852: <http://trac.webkit.org/changeset/175852>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Looks like a 1.7% improvement on DoYouEvenBench on the bots:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D&zoom=%5B1415546583612.4739%2C1415673794361.26%5D

[WebKit Commit Bot]

Re-opened since this is blocked by bug 138626

[Chris Dumez]

Created attachment 241384 [details]
Patch

[Chris Dumez]

*** Bug 138617 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Comment on attachment 241384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241384&action=review

> Source/WebCore/dom/Element.cpp:1244
> +    parserDidSetAttributes();

We now call this function after calling attributeChanged() to make sure that we initialize m_inputType as early as possible. There is now no risk of m_inputType not being initialized when attributeChanged() is called.
Also, we now only need to initialize m_inputType in the override of parserDidSetAttributes() (no longer need to do it in parseAttributes() as well), which is nicer.

[Chris Dumez]

I have just verified Membuster locally and this new patch does not seem to cause any regression. My guess is that the previous patch showed a regression on Membuster because of the crash.

[Chris Dumez]

Comment on attachment 241384 [details]
Patch

Ryosuke, can you please take another look?

[Chris Dumez]

Comment on attachment 241384 [details]
Patch

Thanks.

[Chris Dumez]

Created attachment 241476 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241476 [details]
Patch

Clearing flags on attachment: 241476

Committed r176069: <http://trac.webkit.org/changeset/176069>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Seems like the bot is reporting a 1.4% improvement on speedometer:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D