Bug 138619 – Speed up HTMLTextFormControlElement::setInnerTextValue() a bit

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Speed up HTMLTextFormControlElement::setInnerTextValue() a bit by:
- Not doing a virtual isTextFormControl() call. Relying on innerTextElement() not returning null is sufficient
- Caching the result of innerTextElement() instead of repeatedly calling that virtual function
- De-virtualizing setFormControlValueMatchesRenderer() / formControlValueMatchesRenderer() as these are never overridden.
- Moving the code constructing the innerTextValue from a TextControlInnerTextElement from innerTextValue() to a new innerTextValueFrom(TextControlInnerTextElement&) inline and call this new function from
  setInnerTextValue() to avoid calling innerTextElement() again making sure it is non-null
- Do the tree traversal from innerTextElement's firstChild instead of from innerTextElement. The innerTextElement is a TextControlInnerTextElement. Therefore, it cannot be an HTMLBRElement or a Text node.

[Chris Dumez]

Created attachment 241366 [details]
Patch

[Darin Adler]

Comment on attachment 241366 [details]
Patch

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

> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
> +static inline String finishText(StringBuilder& result)

Now that you moved this function, I noticed that it does not have a great name. Maybe finishInnerTextValue?

> Source/WebCore/html/HTMLTextFormControlElement.cpp:533
> +static inline String innerTextValueFrom(TextControlInnerTextElement& innerText)

Do we really need to inline this? It seems like there's enough code here that the function call overhead is not excessive.

[Chris Dumez]

Comment on attachment 241366 [details]
Patch

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

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
>> +static inline String finishText(StringBuilder& result)
> 
> Now that you moved this function, I noticed that it does not have a great name. Maybe finishInnerTextValue?

How about void stripTrailingNewLineCharacter(StringBuilder&) ? and move the result.toString() to the callers? It is clearer IMHO than "finish*". Finish doesn't really tell you what the function does.

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:533
>> +static inline String innerTextValueFrom(TextControlInnerTextElement& innerText)
> 
> Do we really need to inline this? It seems like there's enough code here that the function call overhead is not excessive.

Hmm. The cost of iterating through the descendants and construct the String probably makes the cost of the function call negligible. I marked it as inline because this code used to be inlined in innerTextValue() and I wanted to make sure I did not cause that one to regress while trying to make setInnerTextValue() faster. I will check if I see any perf difference, if there isn't then I'll drop the inline.

[Chris Dumez]

Comment on attachment 241366 [details]
Patch

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

>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
>>> +static inline String finishText(StringBuilder& result)
>> 
>> Now that you moved this function, I noticed that it does not have a great name. Maybe finishInnerTextValue?
> 
> How about void stripTrailingNewLineCharacter(StringBuilder&) ? and move the result.toString() to the callers? It is clearer IMHO than "finish*". Finish doesn't really tell you what the function does.

strip or trim, hard for me to tell which one would be better here.

[Darin Adler]

Comment on attachment 241366 [details]
Patch

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

>>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
>>>> +static inline String finishText(StringBuilder& result)
>>> 
>>> Now that you moved this function, I noticed that it does not have a great name. Maybe finishInnerTextValue?
>> 
>> How about void stripTrailingNewLineCharacter(StringBuilder&) ? and move the result.toString() to the callers? It is clearer IMHO than "finish*". Finish doesn't really tell you what the function does.
> 
> strip or trim, hard for me to tell which one would be better here.

I think I included to the toString originally because I was trying to share as much code as possible.

I think that "strip", "trim", or "remove" would all be OK.

[Darin Adler]

Comment on attachment 241366 [details]
Patch

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

>>>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
>>>>> +static inline String finishText(StringBuilder& result)
>>>> 
>>>> Now that you moved this function, I noticed that it does not have a great name. Maybe finishInnerTextValue?
>>> 
>>> How about void stripTrailingNewLineCharacter(StringBuilder&) ? and move the result.toString() to the callers? It is clearer IMHO than "finish*". Finish doesn't really tell you what the function does.
>> 
>> strip or trim, hard for me to tell which one would be better here.
> 
> I think I included to the toString originally because I was trying to share as much code as possible.
> 
> I think that "strip", "trim", or "remove" would all be OK.

But it’s "newline character", not "new line character", so it should be stripTrailingNewlineCharacter.

[Darin Adler]

Comment on attachment 241366 [details]
Patch

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

>>>>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
>>>>>> +static inline String finishText(StringBuilder& result)
>>>>> 
>>>>> Now that you moved this function, I noticed that it does not have a great name. Maybe finishInnerTextValue?
>>>> 
>>>> How about void stripTrailingNewLineCharacter(StringBuilder&) ? and move the result.toString() to the callers? It is clearer IMHO than "finish*". Finish doesn't really tell you what the function does.
>>> 
>>> strip or trim, hard for me to tell which one would be better here.
>> 
>> I think I included to the toString originally because I was trying to share as much code as possible.
>> 
>> I think that "strip", "trim", or "remove" would all be OK.
> 
> But it’s "newline character", not "new line character", so it should be stripTrailingNewlineCharacter.

I think stripTrailingNewline would be a fine name.

[Darin Adler]

Comment on attachment 241366 [details]
Patch

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

>>>>>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
>>>>>>> +static inline String finishText(StringBuilder& result)
>>>>>> 
>>>>>> Now that you moved this function, I noticed that it does not have a great name. Maybe finishInnerTextValue?
>>>>> 
>>>>> How about void stripTrailingNewLineCharacter(StringBuilder&) ? and move the result.toString() to the callers? It is clearer IMHO than "finish*". Finish doesn't really tell you what the function does.
>>>> 
>>>> strip or trim, hard for me to tell which one would be better here.
>>> 
>>> I think I included to the toString originally because I was trying to share as much code as possible.
>>> 
>>> I think that "strip", "trim", or "remove" would all be OK.
>> 
>> But it’s "newline character", not "new line character", so it should be stripTrailingNewlineCharacter.
> 
> I think stripTrailingNewline would be a fine name.

And inlining the function seems a little strange. The process of building the string is doing memory allocation, so function call overhead should be negligible by comparison; code sharing would be better.

[Chris Dumez]

Created attachment 241427 [details]
Patch

[Chris Dumez]

Thanks, I renamed to stripTrailingNewline() and dropped the inline.

[Chris Dumez]

Created attachment 241435 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241435 [details]
Patch

Clearing flags on attachment: 241435

Committed r176030: <http://trac.webkit.org/changeset/176030>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.