Share locals between members

Share locals between members

R=efortuna@google.com

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

[Johnni Winther, 2017-08-17 12:36:59]

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

[Emily Fortuna, 2017-08-17 19:01:59]

I find the number of new crashes a little concerning. What's the error that
you're seeing with these new crashes? If you run Siggi's script, it puts a
preview of the error in the status file.

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
File pkg/compiler/lib/src/js_model/closure.dart (left):

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
pkg/compiler/lib/src/js_model/closure.dart:405: final KernelToLocalsMap
localsMap;
if we get rid of this field, all JClosureClasses can just become JClasses,
because there's no need to have a separate type of object.

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
File pkg/compiler/lib/src/js_model/closure.dart (right):

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
pkg/compiler/lib/src/js_model/closure.dart:142:
_globalLocalsMap.setLocalsMap(closureClass.callMethod, localsMap);
what about grouping the locals map for all the created closures in ScopeModel
(like the capturedScopes)?

[Johnni Winther, 2017-08-18 14:59:39]

PTAL - I updated the status files using a Windows-ified version of the script.

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
File pkg/compiler/lib/src/js_model/closure.dart (left):

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
pkg/compiler/lib/src/js_model/closure.dart:405: final KernelToLocalsMap
localsMap;
On 2017/08/17 19:01:59, Emily Fortuna wrote:
> if we get rid of this field, all JClosureClasses can just become JClasses,
> because there's no need to have a separate type of object. 

It does have a better to `toString` and the right [isClosure] value!

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
File pkg/compiler/lib/src/js_model/closure.dart (right):

https://codereview.chromium.org/2995113002/diff/1/pkg/compiler/lib/src/js_mod...
pkg/compiler/lib/src/js_model/closure.dart:142:
_globalLocalsMap.setLocalsMap(closureClass.callMethod, localsMap);
On 2017/08/17 19:01:59, Emily Fortuna wrote:
> what about grouping the locals map for all the created closures in ScopeModel
> (like the capturedScopes)?

This line means we are sharing the map for all those closures. (Not sure what
you mean by 'grouping')

[Johnni Winther, 2017-08-19 07:32:45]

ping

[Emily Fortuna, 2017-08-19 16:05:00]

lgtm

[Johnni Winther, 2017-08-21 13:02:17]

Description was changed from

==========
Share locals between members
==========

to

==========
Share locals between members

R=efortuna@google.com

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

[Johnni Winther, 2017-08-21 13:02:18]

Committed patchset #3 (id:40001) manually as
1658036b05a7afc83f692b3ff7631926780a36bc (presubmit successful).