Bug 95924 – Deploy StringBuilder in more places in WebKit2

• Component: New Bugs
• Status: RESOLVED FIXED

[Jinwoo Song]

Deploy StringBuilder to concatenate strings more efficiently.

[Jinwoo Song]

Created attachment 162394 [details]
patch

[kov's GTK+ EWS bot]

Comment on attachment 162394 [details]
patch

Attachment 162394 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13772121

[Build Bot]

Comment on attachment 162394 [details]
patch

Attachment 162394 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13772125

[Build Bot]

Comment on attachment 162394 [details]
patch

Attachment 162394 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13774119

[Jinwoo Song]

Created attachment 162398 [details]
patch

[Build Bot]

Comment on attachment 162398 [details]
patch

Attachment 162398 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13772137

[Jinwoo Song]

Created attachment 162403 [details]
patch. build error fix.

[Build Bot]

Comment on attachment 162403 [details]
patch. build error fix.

Attachment 162403 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13768206

[Build Bot]

Comment on attachment 162403 [details]
patch. build error fix.

Attachment 162403 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13765458

[Ryuan Choi]

Comment on attachment 162403 [details]
patch. build error fix.

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

> Source/WebKit2/Shared/WebMemorySampler.cpp:141
> +    processDetails.appendLiteral("\n");

append('\n') looks better.

> Source/WebKit2/Shared/WebMemorySampler.cpp:142
> +    writeToFile(m_sampleLogFile, processDetails.toString().utf8().data(), processDetails.toString().utf8().length());

I am not sure, but isn't it better to keep String reference instead of using toString twice ?

> Source/WebKit2/Shared/WebMemorySampler.cpp:153
> +    header.appendLiteral("\n");
> +    writeToFile(m_sampleLogFile, header.toString().utf8().data(), header.toString().utf8().length());

Ditto.

> Source/WebKit2/Shared/WebMemorySampler.cpp:184
> +    statString.appendLiteral("\n");
> +    writeToFile(m_sampleLogFile, statString.toString().utf8().data(), statString.toString().utf8().length());

Ditto.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:127
> +    etringBuilder result;

Typo

[Early Warning System Bot]

Comment on attachment 162403 [details]
patch. build error fix.

Attachment 162403 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13765470

[Jinwoo Song]

Created attachment 162414 [details]
fixed typo. :(

[Jinwoo Song]

Created attachment 162419 [details]
new patch. Applied ryuan's comments.

[Jinwoo Song]

Created attachment 162421 [details]
patch.

[Jinwoo Song]

(In reply to comment #10)
> (From update of attachment 162403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162403&action=review
> 
> > Source/WebKit2/Shared/WebMemorySampler.cpp:141
> > +    processDetails.appendLiteral("\n");
> 
> append('\n') looks better.
done.

> 
> > Source/WebKit2/Shared/WebMemorySampler.cpp:142
> > +    writeToFile(m_sampleLogFile, processDetails.toString().utf8().data(), processDetails.toString().utf8().length());
> 
> I am not sure, but isn't it better to keep String reference instead of using toString twice ?
done. 
> 
> > Source/WebKit2/Shared/WebMemorySampler.cpp:153
> > +    header.appendLiteral("\n");
> > +    writeToFile(m_sampleLogFile, header.toString().utf8().data(), header.toString().utf8().length());
> 
> Ditto.
done.

> 
> > Source/WebKit2/Shared/WebMemorySampler.cpp:184
> > +    statString.appendLiteral("\n");
> > +    writeToFile(m_sampleLogFile, statString.toString().utf8().data(), statString.toString().utf8().length());
> 
> Ditto.
done.

> 
> > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:127
> > +    etringBuilder result;
> 
> Typo
done.

[Ryuan Choi]

Comment on attachment 162421 [details]
patch.

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

It looks almost fine to me.

Below are some nit.

> Source/WebKit2/ChangeLog:9
> + 

one space.

> Source/WebKit2/Shared/WebMemorySampler.cpp:49
> +    : m_separator(ASCIILiteral("\t"))  

remove spaces at end of lien.

> Source/WebKit2/Shared/WebMemorySampler.cpp:151
>              header.append(stats.keys[i].utf8().data());

stats.keys[i] looks enough.

> Source/WebKit2/Shared/WebMemorySampler.cpp:176
> +    StringBuilder statString; 

space.

> Source/WebKit2/Shared/WebMemorySampler.cpp:187
> +    statString.append('\n');
> +    

ditto.

[Benjamin Poulain]

Comment on attachment 162421 [details]
patch.

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

All the changes are good ideas, but we can do even better. Some comments:

>> Source/WebKit2/Shared/WebMemorySampler.cpp:49
>> -    : m_separator(String("\t"))  
>> +    : m_separator(ASCIILiteral("\t"))  
> 
> remove spaces at end of lien.

Reading WebMemorySampler, m_separator is a bad idea, you should get rid of it.

You can just have
const char separator = '\t'; in the cpp file.

> Source/WebKit2/Shared/WebMemorySampler.cpp:141
> -    String processDetails = String("Process: ");
> +    StringBuilder processDetails;
> +    processDetails.appendLiteral("Process: ");
>      processDetails.append(processName());
> -    processDetails.append(String("\n"));
> -    writeToFile(m_sampleLogFile, processDetails.utf8().data(), processDetails.utf8().length());
> +    processDetails.append('\n');

Since there is no branch, it is more interesting to use the String Operators:
    String processDetails = "Process: " + processName() + '\n';

> Source/WebKit2/Shared/WebMemorySampler.cpp:186
>              statString.append(String::number(memoryStats.values[i]));
>          }
>      }
> -    statString.append(String("\n"));
> -    writeToFile(m_sampleLogFile, statString.utf8().data(), statString.utf8().length());
> +    statString.append('\n');

This one is a big win with StringBuilder.

> Source/WebKit2/WebProcess/WebCoreSupport/WebContextMenuClient.cpp:87
> -    String url("http://www.google.com/search?q=");
> +    StringBuilder url;
> +    url.appendLiteral("http://www.google.com/search?q=");
>      url.append(encoded);
> -    url.append("&ie=UTF-8&oe=UTF-8");
> +    url.appendLiteral("&ie=UTF-8&oe=UTF-8");

This should use String Operators:
    url = "http://www.google.com/search?q=" + encoded + "&ie=UTF-8&oe=UTF-8";

[Jinwoo Song]

Created attachment 162637 [details]
patch applied comments from Ryuan and Benjamin

[Jinwoo Song]

Created attachment 162640 [details]
add ChangeLog

Applied comments by Ryuan and Benjamin. Thanks for scrupulous review!

[Ryuan Choi]

Comment on attachment 162637 [details]
patch applied comments from Ryuan and Benjamin

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

I think that this patch can be landed?

Why don't you add changelog and r?

> Source/WebKit2/Shared/WebMemorySampler.cpp:39
> +const char separator = '\t';

nit. static const char?

[Jinwoo Song]

Created attachment 162649 [details]
patch.

[Benjamin Poulain]

> > Source/WebKit2/Shared/WebMemorySampler.cpp:39
> > +const char separator = '\t';
> 
> nit. static const char?

Out of curiosity, why?

[Ryuan Choi]

(In reply to comment #22)
> > > Source/WebKit2/Shared/WebMemorySampler.cpp:39
> > > +const char separator = '\t';
> > 
> > nit. static const char?
> 
> Out of curiosity, why?

Just to clarify the scope.
It will be same irrespective of 'static'

Am I right?

[Benjamin Poulain]

> > Out of curiosity, why?
> 
> Just to clarify the scope.
> It will be same irrespective of 'static'
> 
> Am I right?

That is right. I was thinking the value will likely end up in the __text segment anyway so why bother with static... But I see your point about making the scope explicit.

[WebKit Review Bot]

Comment on attachment 162649 [details]
patch.

Clearing flags on attachment: 162649

Committed r127828: <http://trac.webkit.org/changeset/127828>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.