dart2js: Avoid escaping in strings if it's not necessary.

dart2js: Avoid escaping in strings if it's not necessary.

BUG= http://dartbug.com/22145
R=johnniwinther@google.com, lrn@google.com, sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43967
Reverted: https://code.google.com/p/dart/source/detail?r=43969

Committed: https://code.google.com/p/dart/source/detail?r=43979
Reverted: https://code.google.com/p/dart/source/detail?r=43981

This CL is obsolete after https://github.com/dart-lang/sdk/commit/dcf576a428eb95aa830fbee7a91006cddc6e8090

I'm marking this issue as closed.

[floitsch, 2015-02-17 13:01:27]

floitsch@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2015-02-17 14:19:46]

lgtm

https://codereview.chromium.org/930263002/diff/20001/tests/compiler/dart2js/s...
File tests/compiler/dart2js/string_escapes2_test.dart (right):

https://codereview.chromium.org/930263002/diff/20001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes2_test.dart:21: generated.contains(r'"Тест
на Кирилица - great.\nNext \"line\"."') ||
Shouldn't it be '\\nNext' instead of '\nNext' ?

[ahe, 2015-02-17 15:55:15]

ahe@google.com changed reviewers:
+ ahe@google.com

[ahe, 2015-02-17 15:55:15]

I don't see any issues related to incremental compilation.

I did have a thought: strings like '"' aren't handled well by this algorithm.
Might be something to consider in the future.

Not saying l-g-t-m as I didn't carefully review the code, but it looks nice. You
may want to ask Lasse to review it, he seems to be good at poking holes in stuff
like this :-)

[sra1, 2015-02-17 20:03:52]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-02-17 20:03:53]

DBC

https://codereview.chromium.org/930263002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (right):

https://codereview.chromium.org/930263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:282: int i = 0;
JsBuilder.escapedString already does this.
It is not quite right (no $LS or $PS), so better to fix it there than make
another copy of escaping code.

I think JsBuilder.escapedString could be reduced to one call to replaceAllMapped
with a static cached RegExp.  That should already be implemented efficiently for
the no-hit case.

The whole body of visitString should just be:

return js.escapedString(constant.primitiveValue.slowToString());

https://codereview.chromium.org/930263002/diff/20001/tests/compiler/dart2js/s...
File tests/compiler/dart2js/string_escapes2_test.dart (right):

https://codereview.chromium.org/930263002/diff/20001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes2_test.dart:18: return
compileExpression("'Тест на Кирилица - great.\\nNext \"line\".'")
Add $PS and $LS.

[floitsch, 2015-02-20 13:55:08]

New patchsets have been uploaded after l-g-t-m from johnniwinther@google.com

[floitsch, 2015-02-20 13:55:50]

PTAL.

https://codereview.chromium.org/930263002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (right):

https://codereview.chromium.org/930263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:282: int i = 0;
On 2015/02/17 20:03:53, sra1 wrote:
> JsBuilder.escapedString already does this.
> It is not quite right (no $LS or $PS), so better to fix it there than make
> another copy of escaping code.
> 
> I think JsBuilder.escapedString could be reduced to one call to
replaceAllMapped
> with a static cached RegExp.  That should already be implemented efficiently
for
> the no-hit case.
> 
> The whole body of visitString should just be:
> 
> return js.escapedString(constant.primitiveValue.slowToString());
> 

Done.

https://codereview.chromium.org/930263002/diff/20001/tests/compiler/dart2js/s...
File tests/compiler/dart2js/string_escapes2_test.dart (right):

https://codereview.chromium.org/930263002/diff/20001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes2_test.dart:18: return
compileExpression("'Тест на Кирилица - great.\\nNext \"line\".'")
On 2015/02/17 20:03:53, sra1 wrote:
> Add $PS and $LS.

Done.

https://codereview.chromium.org/930263002/diff/20001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes2_test.dart:21: generated.contains(r'"Тест
на Кирилица - great.\nNext \"line\"."') ||
On 2015/02/17 14:19:46, Johnni Winther wrote:
> Shouldn't it be '\\nNext' instead of '\nNext' ?

No. This string is a raw string.

[Lasse Reichstein Nielsen, 2015-02-23 13:25:04]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2015-02-23 13:25:05]

lgtm

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/builder.dart (right):

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:300: String escaped =
value.replaceAll('\\', '\\\\');
Why not combine the backslash-escaping in the other replaceAllMapped?

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:324: // string literal anyway.
Technically, you don't have to escape \b, \t, \f and \v. They are not line
terminator, so they are allowed in output. It's only for readability, not size,
that they are escaped here.
It's probably still a good idea to spare an extra byte per instance for
readability.

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:337: });
Does this have a measurable performance impact? Because it does look slightly
slow creating one-character strings for each match and doing a string-switch
instead of looking at the code-unit directly.

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:340: // into ". Verify that assumption.
Have you considered checking whether switching to ' instead of " gives a shorter
program? Or even doing it per string being escaped?

[floitsch, 2015-02-23 16:56:32]

New patchsets have been uploaded after l-g-t-m from lrn@google.com

[floitsch, 2015-02-23 16:56:41]

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/builder.dart (right):

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:300: String escaped =
value.replaceAll('\\', '\\\\');
On 2015/02/23 13:25:05, Lasse Reichstein Nielsen wrote:
> Why not combine the backslash-escaping in the other replaceAllMapped?
>  

Done.

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:324: // string literal anyway.
On 2015/02/23 13:25:05, Lasse Reichstein Nielsen wrote:
> Technically, you don't have to escape \b, \t, \f and \v. They are not line
> terminator, so they are allowed in output. It's only for readability, not
size,
> that they are escaped here.
> It's probably still a good idea to spare an extra byte per instance for
> readability.

Yes. My initial patch (that wasn't using this function) didn't encode it. But I
think it's cleaner this way too.

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:337: });
On 2015/02/23 13:25:05, Lasse Reichstein Nielsen wrote:
> Does this have a measurable performance impact? Because it does look slightly
> slow creating one-character strings for each match and doing a string-switch
> instead of looking at the code-unit directly.

I tried with a string of 500000 \u2029 characters (probably the worst-case
scenario) and didn't see any difference in performance.

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:340: // into ". Verify that assumption.
On 2015/02/23 13:25:05, Lasse Reichstein Nielsen wrote:
> Have you considered checking whether switching to ' instead of " gives a
shorter
> program? Or even doing it per string being escaped?

Of course. But I didn't yet go ahead and to the optimization.
(I think per string would be easier).

[sra1, 2015-02-23 17:47:27]

lgtm

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/builder.dart (right):

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:324: // string literal anyway.
On 2015/02/23 13:25:05, Lasse Reichstein Nielsen wrote:
> Technically, you don't have to escape \b, \t, \f and \v. They are not line
> terminator, so they are allowed in output. It's only for readability, not
size,
> that they are escaped here.
> It's probably still a good idea to spare an extra byte per instance for
> readability.

I think we can have more than one version of this method that can be used
depending on context.
For example, a version that omits '\t' -> r'\t' might be called from the
constant emitter in minified mode.

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:326: new
RegExp('\n|"|\b|\t|\v|\u2028|\u2029'), (match) {
new RegExp -> static variable.

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:340: // into ". Verify that assumption.
On 2015/02/23 16:56:41, floitsch wrote:
> On 2015/02/23 13:25:05, Lasse Reichstein Nielsen wrote:
> > Have you considered checking whether switching to ' instead of " gives a
> shorter
> > program? Or even doing it per string being escaped?
> 
> Of course. But I didn't yet go ahead and to the optimization.
> (I think per string would be easier).

This has an issue :-)
https://code.google.com/p/dart/issues/detail?id=22369

[floitsch, 2015-02-23 18:16:17]

New patchsets have been uploaded after l-g-t-m from sra@google.com

[floitsch, 2015-02-23 18:16:45]

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/builder.dart (right):

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:326: new
RegExp('\n|"|\b|\t|\v|\u2028|\u2029'), (match) {
On 2015/02/23 17:47:27, sra1 wrote:
> new RegExp -> static variable.

It reads less nice, but done.
Called "_stringEscapeRegExp".

[floitsch, 2015-02-23 18:37:06]

Committed patchset #6 (id:100001) manually as 43967 (presubmit successful).

[floitsch, 2015-02-23 18:59:38]

Turns out the original code wasn't handling \r and I didn't check for it.

Had to revert, and will land the updated one tomorrow.

[floitsch, 2015-02-24 09:50:21]

Committed patchset #8 (id:140001) manually as 43979 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-02-24 14:14:21]

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/builder.dart (right):

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:326: new
RegExp('\n|"|\b|\t|\v|\u2028|\u2029'), (match) {
How about:
  new RegExp(r'[\n\b\t\v"\u2028\u2029]')
instead? I don't think it's any faster, but it is slightly more idiomatic when
you are switching on single characters.

[floitsch, 2015-02-24 20:49:44]

The reason for the buildbot failure was the test-server didn't specify that the
JS files are in UTF-8 and the browser assumed that they were iso-latin1 (or
similar).

Fix is underway: https://chromiumcodereview.appspot.com/950353004/

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/builder.dart (right):

https://codereview.chromium.org/930263002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/builder.dart:326: new
RegExp('\n|"|\b|\t|\v|\u2028|\u2029'), (match) {
On 2015/02/24 14:14:21, Lasse Reichstein Nielsen wrote:
> How about:
>   new RegExp(r'[\n\b\t\v"\u2028\u2029]')
> instead? I don't think it's any faster, but it is slightly more idiomatic when
> you are switching on single characters.

Done.

[sra1, 2015-12-17 18:48:18]

Description was changed from

==========
dart2js: Avoid escaping in strings if it's not necessary.

BUG= http://dartbug.com/22145
R=johnniwinther@google.com, lrn@google.com, sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43967
Reverted: https://code.google.com/p/dart/source/detail?r=43969

Committed: https://code.google.com/p/dart/source/detail?r=43979
Reverted: https://code.google.com/p/dart/source/detail?r=43981
==========

to

==========
dart2js: Avoid escaping in strings if it's not necessary.

BUG= http://dartbug.com/22145
R=johnniwinther@google.com, lrn@google.com, sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43967
Reverted: https://code.google.com/p/dart/source/detail?r=43969

Committed: https://code.google.com/p/dart/source/detail?r=43979
Reverted: https://code.google.com/p/dart/source/detail?r=43981

This CL is obsolete after
https://github.com/dart-lang/sdk/commit/dcf576a428eb95aa830fbee7a91006cddc6e8090

I'm marking this issue as closed.
==========