Improve performance of dart2js StringBuffer.

Improve performance of dart2js StringBuffer.

Committed: https://code.google.com/p/dart/source/detail?r=17639

[ahe, 2012-12-14 14:54:26]

Before:

StringInterpolation(RunTime): 8559.82905982906 us.
StringBuffer(RunTime): 23022.988505747126 us.
StringIdiomatic(RunTime): 22494.38202247191 us.
BuildTableInterpolation(RunTime): 25.304604172729228 us.
BuildTableBuffer(RunTime): 65.72893387669252 us.
BuildTableIdiomatic(RunTime): 65.66634927931182 us.

After:

StringInterpolation(RunTime): 8504.237288135593 us.
StringBuffer(RunTime): 9833.333333333334 us.
StringIdiomatic(RunTime): 9800 us.
BuildTableInterpolation(RunTime): 24.653920589721782 us.
BuildTableBuffer(RunTime): 27.020089436496036 us.
BuildTableIdiomatic(RunTime): 27.03469903621298 us.

Pure JavaScript (target):

StringInterpolation(RunTime): 5109.693877551021 us.
StringBuffer(RunTime): 6863.013698630137 us.
StringIdiomatic(RunTime): 5202.597402597403 us.
BuildTableInterpolation(RunTime): 11.499407780499304 us.
BuildTableBuffer(RunTime): 13.099893235870127 us.
BuildTableIdiomatic(RunTime): 11.432948614612451 us.

StringIdiomatic goes from 23% to 53% of JavaScript.
BuildTableIdiomatic goes from 17% to 47% of JavaScript.

[ngeoffray, 2012-12-19 16:23:29]

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart
(right):

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:144:
JS('String', '# += #', _contents, obj);
Make this JS call a helper?

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:155:
JS('String', '# += #', _contents, str);
str not being a string I wonder what happens here. Not sure we want [Object
object] being written in _contents. What was the previous behavior?
NoSuchMethod?

[Lasse Reichstein Nielsen, 2012-12-19 17:02:42]

drive-by comment

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart
(right):

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:147:
String str = obj.toString();
If you just do 
 String str = "$obj";
here, you'll throw an appropriate error if obj.toString doesn't return a string.

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:152: if
(str == null || str.isEmpty) {
I think it's fair to throw at this point. We can't concatenate a string with a
non-string, and you get one chance to return something reasonable from toString.
Also, there is no reason to believe that str will have an isEmpty getter if it's
not a string.

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:155:
JS('String', '# += #', _contents, str);
It'll do an implicit JS conversion to String. That's probably never correct.

[sra1, 2012-12-19 17:09:51]

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart
(right):

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:144:
JS('String', '# += #', _contents, obj);
On 2012/12/19 16:23:29, ngeoffray wrote:
> Make this JS call a helper?

It is a misuse of JS to have # be an l-value.
That would constrain the expression in ways that are not checked, e.g. what if
_contents compiles to  this.get$_contents()?

Use:

_contents = JS('String', '# + #', _contents, obj);

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:152: if
(str == null || str.isEmpty) {
isEmpty is not an empty string check here because of the previous if-statement.
Do we really want to silently accept a toString that returns an empty List or
Map?

I suggest replacing most of the body here with:

_contents = JS('String', '# + #', _contents, '$obj');

And making the VM version have consistent behaviour (i.e. also make it use
interpolation for checking the result of toString).

[ahe, 2013-01-02 10:20:42]

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart
(right):

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:144:
JS('String', '# += #', _contents, obj);
On 2012/12/19 16:23:29, ngeoffray wrote:
> Make this JS call a helper?

That's measurably slower on Firefox.

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:144:
JS('String', '# += #', _contents, obj);
On 2012/12/19 17:09:51, sra1 wrote:
> On 2012/12/19 16:23:29, ngeoffray wrote:
> > Make this JS call a helper?
> 
> It is a misuse of JS to have # be an l-value.
> That would constrain the expression in ways that are not checked, e.g. what if
> _contents compiles to  this.get$_contents()?
> 
> Use:
> 
> _contents = JS('String', '# + #', _contents, obj);

Done.

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:147:
String str = obj.toString();
On 2012/12/19 17:02:42, Lasse Reichstein Nielsen wrote:
> If you just do 
>  String str = "$obj";
> here, you'll throw an appropriate error if obj.toString doesn't return a
string.

Both Lars and I have changed the semantics of StringBuffer.add in separate CLs.
See:

https://codereview.chromium.org/11618037 (make the semantics equivalent to
'$obj').

https://codereview.chromium.org/11645019 (don't return this)

I'll have to rebase this CL before addressing the remaining comments.

https://codereview.chromium.org/11565034/diff/7/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:155:
JS('String', '# += #', _contents, str);
On 2012/12/19 16:23:29, ngeoffray wrote:
> str not being a string I wonder what happens here. Not sure we want [Object
> object] being written in _contents. What was the previous behavior?
> NoSuchMethod?

It would throw an ArgumentError from String.concat, but not until calling
toString on the string buffer. I have temporarily changed it to eagerly force
the same error, but I'll change this after rebasing this CL. The semantics have
been cleaned up, see:
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/test...

[ahe, 2013-01-02 13:43:14]

PTAL.

If you want to look at the incremental changes, look at patch set 2 and patch
set 4.  However, the full diff is probably just as easy to read.

Cheers,
Peter

https://codereview.chromium.org/11565034/diff/15001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11565034/diff/15001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:336: String
S(value) {
This change has a big impact as well. Below, the "after" numbers are
followed by a percentage.  Larger than 100% means an improvement.

Before dart2js

StringInterpolation(RunTime): 8679.65367965368 us.
StringBuffer(RunTime): 10329.896907216495 us.
StringIdiomatic(RunTime): 10350.515463917525 us.
BuildTableInterpolation(RunTime): 26.33415408113553 us.
BuildTableBuffer(RunTime): 28.16623713155041 us.
BuildTableIdiomatic(RunTime): 28.1864817633463 us.

Before jsshell (Firefox)

StringInterpolation(RunTime): 109157.8947368421 us.
StringBuffer(RunTime): 226111.11111111112 us.
StringIdiomatic(RunTime): 227777.77777777778 us.
BuildTableInterpolation(RunTime): 820.3445447087777 us.
BuildTableBuffer(RunTime): 969.9321047526673 us.
BuildTableIdiomatic(RunTime): 968.054211035818 us.

After dart2js

StringInterpolation(RunTime): 7715.384615384615 us. (112%)
StringBuffer(RunTime): 9076.923076923076 us. (114%)
StringIdiomatic(RunTime): 9109.09090909091 us. (114%)
BuildTableInterpolation(RunTime): 24.186428994691077 us. (109%)
BuildTableBuffer(RunTime): 27.256125814276757 us. (103%)
BuildTableIdiomatic(RunTime): 27.18758071313024 us. (104%)

After jsshell

StringInterpolation(RunTime): 49560.9756097561 us. (220%)
StringBuffer(RunTime): 176000 us. (128%)
StringIdiomatic(RunTime): 175416.66666666666 us. (130%)
BuildTableInterpolation(RunTime): 745.4342154304883 us. (110%)
BuildTableBuffer(RunTime): 938.9671361502348 us. (103%)
BuildTableIdiomatic(RunTime): 944.2870632672333 us. (103%)

Compare to JavaScript version.  Percentage numbers are relative to
"after" numbers above.  For example, StringIdiomatic is almost twice
as fast in hand-written JavaScript than the code dart2js generates
when running on V8, and it is much worse when running on jsshell (Firefox).

V8:

StringInterpolation(RunTime): 5273.684210526316 us. (68%)
StringBuffer(RunTime): 7006.993006993007 us. (77%)
StringIdiomatic(RunTime): 5271.0526315789475 us. (58%)
BuildTableInterpolation(RunTime): 11.748258320703956 us. (49%)
BuildTableBuffer(RunTime): 13.40931947703654 us. (49%)
BuildTableIdiomatic(RunTime): 11.786337277827542 us. (43%)

jsshell:

StringInterpolation(RunTime): 31952.380952380954 us. (64%)
StringBuffer(RunTime): 80160 us. (46%)
StringIdiomatic(RunTime): 32047.619047619046 us. (18%)
BuildTableInterpolation(RunTime): 53.917075537822825 us. (7%)
BuildTableBuffer(RunTime): 191.71779141104295 us. (20%)
BuildTableIdiomatic(RunTime): 52.4893053040443 us. (6%)

[ngeoffray, 2013-01-03 13:56:54]

LGTM!

https://codereview.chromium.org/11565034/diff/15001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11565034/diff/15001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:223: (content
is String) ? content : '$content');
Why not leave this check to JsStringBuffer? Decoupling this check seems like
error prone.

[ahe, 2013-01-03 15:42:09]

Thank you!

https://codereview.chromium.org/11565034/diff/15001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11565034/diff/15001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:223: (content
is String) ? content : '$content');
On 2013/01/03 13:56:54, ngeoffray wrote:
> Why not leave this check to JsStringBuffer? Decoupling this check seems like
> error prone.

Good idea. I also added a test to ensure that: new StringBuffer(obj).toString()
and (new StringBuffer()..add(obj)).toString() behaves the same.

[ahe, 2013-01-25 15:31:36]

A few minor tweaks after updating to lib_v2 and internal libraries.

[ngeoffray, 2013-01-28 09:35:20]

LGTM