dart2js allocation tweaks

dart2js allocation tweaks

In compiling a large app:

50% reduction in number of strings allocated.
20% reduction in number of closures allocated.

It is hard to tell if there is a performance improvement because it is in the noise.

R=kmillikin@google.com, sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/dc8a4f21b8f32b4bfd73b51e86412b27a2c0f45d

[sra1, 2017-05-05 00:28:50]

Description was changed from

==========
dart2js allocation tweaks

50% reduction in number of strings allocated.
20% reduction in number of closures allocated.
==========

to

==========
dart2js allocation tweaks

50% reduction in number of strings allocated.
20% reduction in number of closures allocated.

It is hard to tell if there is a performance improvement because it is in the
noise.
==========

[sra1, 2017-05-05 01:10:56]

Patchset #1 (id:1) has been deleted

[sra1, 2017-05-05 01:20:20]

Description was changed from

==========
dart2js allocation tweaks

50% reduction in number of strings allocated.
20% reduction in number of closures allocated.

It is hard to tell if there is a performance improvement because it is in the
noise.
==========

to

==========
dart2js allocation tweaks

In compiling a large app:

50% reduction in number of strings allocated.
20% reduction in number of closures allocated.

It is hard to tell if there is a performance improvement because it is in the
noise.
==========

[sra1, 2017-05-05 01:20:20]

sra@google.com changed reviewers:
+ ahe@google.com, sigmund@google.com

[ahe, 2017-05-05 05:27:44]

https://codereview.chromium.org/2864573002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2864573002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4374: RangeError.checkValueInInterval(line, 1,
lineStarts.length, 'line');
Did you change the error message produced by RangeError.checkValueInInterval?

[sra1, 2017-05-05 05:49:01]

https://codereview.chromium.org/2864573002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2864573002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4374: RangeError.checkValueInInterval(line, 1,
lineStarts.length, 'line');
On 2017/05/05 05:27:44, ahe wrote:
> Did you change the error message produced by RangeError.checkValueInInterval?

No. I also think the RangeError description is confusing, but it is still
confusing with this interpolated string repeating things that are printed with
the message.

I would be supportive of an independent change to improve the corelib errors.

[Siggi Cherem (dart-lang), 2017-05-05 17:39:08]

sigmund@google.com changed reviewers:
+ kmillikin@google.com

[Siggi Cherem (dart-lang), 2017-05-05 17:39:08]

+kmillikin

dart2js changes lgtm.

At least for dart2js it seems like the kernel/ast.dart changes only affect
internal errors, and not errors that are presented to our users. Is that also
the case for other users of Source elsewhere?

In that case, I'm happy with this change: it will be slightly a more cryptic
error message, but we'll reduce the cost of compilation when we don't hit the
non-error cases.

[Kevin Millikin (Google), 2017-05-05 17:44:15]

LGTM.

[sra1, 2017-05-05 21:40:51]

Description was changed from

==========
dart2js allocation tweaks

In compiling a large app:

50% reduction in number of strings allocated.
20% reduction in number of closures allocated.

It is hard to tell if there is a performance improvement because it is in the
noise.
==========

to

==========
dart2js allocation tweaks

In compiling a large app:

50% reduction in number of strings allocated.
20% reduction in number of closures allocated.

It is hard to tell if there is a performance improvement because it is in the
noise.

R=kmillikin@google.com, sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/dc8a4f21b8f32b4bfd73b51e86412b27a2c0f45d
==========

[sra1, 2017-05-05 21:40:52]

Committed patchset #1 (id:20001) manually as
dc8a4f21b8f32b4bfd73b51e86412b27a2c0f45d (presubmit successful).

[ahe, 2017-05-08 10:25:21]

https://codereview.chromium.org/2864573002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2864573002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4374: RangeError.checkValueInInterval(line, 1,
lineStarts.length, 'line');
On 2017/05/05 05:49:01, sra1 wrote:
> On 2017/05/05 05:27:44, ahe wrote:
> > Did you change the error message produced by
RangeError.checkValueInInterval?
> 
> No. I also think the RangeError description is confusing, but it is still
> confusing with this interpolated string repeating things that are printed with
> the message.
> 
> I would be supportive of an independent change to improve the corelib errors.

I'm surprised that this change was submitted although I had expressed concerns
about it and suggested ways to work around it.