| Summary: | Speed up HTMLTextFormControlElement::setInnerTextValue() a bit | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, darin, kling, rniwa | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Dumez
2014-11-11 11:55:18 PST
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. |