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.
Created attachment 241366 [details] Patch
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.
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.
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.
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.
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.
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.
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.
Created attachment 241427 [details] Patch
Thanks, I renamed to stripTrailingNewline() and dropped the inline.
Created attachment 241435 [details] Patch
Comment on attachment 241435 [details] Patch Clearing flags on attachment: 241435 Committed r176030: <http://trac.webkit.org/changeset/176030>
All reviewed patches have been landed. Closing bug.