Turn the VM's dart:typed_data into a patch

Turn the VM's dart:typed_data into a patch

Before, the VM's dart:typed_data was a complete replacement of the SDK's
dart:typed_data implementation instead of a patch.  This is unlike all
the other SDK libraries.  This difference requires special-casing for
dart:typed_data in tools that handle the SDK libraries (e.g., the
Analyzer's patching support, the GN build).

This change makes dart:typed_data back into a patch to the SDK's
implementation.  It reintroduces a distinction between abstract
interface and concrete implementation classes, so there are more
classes.

BUG=
R=fschneider@google.com, vegorov@google.com

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

[Kevin Millikin (Google), 2016-12-15 14:27:04]

kmillikin@google.com changed reviewers:
+ fschneider@google.com, vegorov@google.com

[Kevin Millikin (Google), 2016-12-15 14:27:05]

Context: I want to use the analyzer-based front end to parse the SDK library
implementations.  I could definitely work around dart:typed_data by hardcoding
that it is special and passing a separate path for it, but that doesn't seem
very scalable.

I tried the approach we discussed earlier (Florian) which was to make the
interface class in the SDK non-abstract and have external (instead of abstract)
methods, but that doesn't work because the VM implementation changes the
superclass (into a mixin application) which can't be done by patching.  The
workaround for that would be to duplicate all the superclass and mixin methods
everywhere they're needed but since the main point seems to be to reduce the
application snapshot size (and that would likely grow it) that seems like a
non-starter.

I only looked at the TypedDataCopy benchmark.  The application snapshot grows by
2 KB over 6 MB (0.004%).  There does not appear to be any change in runtime.

https://codereview.chromium.org/2571563005/diff/1/runtime/vm/method_recognizer.h
File runtime/vm/method_recognizer.h (right):

https://codereview.chromium.org/2571563005/diff/1/runtime/vm/method_recognize...
runtime/vm/method_recognizer.h:546: V(_ListFactory, _List, ., kArrayCid,
0x63078b15)                             \
This macro now (redundantly) includes the class name, the constructor name, and
the symbol which is their concatenation.  We need the class and constructor
names separately because the fingerprint checker looks up the class by name and
then looks up the constructor by name in the class.

Before, it used the cid of the result to get the class containing the
constructor, but that connection is broken (the interface class contains the
factory constructor, but the result type is the implementation class).

https://codereview.chromium.org/2571563005/diff/1/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/2571563005/diff/1/runtime/vm/symbols.h#newcod...
runtime/vm/symbols.h:234: V(Float64List, "_Float64List")                        
                      \
The association between the C++ identifier and the string for symbols is not
systematic at all.  The identifier names should probably include a leading
underscore here.

In the very next line there is a leading underscore in the identifier, but not
in the string.  That leads me to think it's random.

[Florian Schneider, 2016-12-15 17:38:00]

General comments/questions:
1. I like the objective of this CL since I'm all for making typed_data a patch
file and sharing as much as possible with the shared SDK file, but I don't think
limitations of the patch mechanism should force us to introduce unnecessary
indirections.

2. Why do you need to  re-introduce _Float32x4, _Int32x4 and _Float64x2? Their
superclass is not changed.

3. Why can't the superclass of the patch-class be different? It seems like a
unnecessary limitation to me. Is this a issue with the analyzer front-end, the
VM or both?

[Florian Schneider, 2016-12-16 22:20:37]

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
File runtime/lib/typed_data_patch.dart (right):

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
runtime/lib/typed_data_patch.dart:1937: class _Float32x4 implements Float32x4 {
@patch class Float32x4 {

should work.

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
runtime/lib/typed_data_patch.dart:1987: class _Int32x4 implements Int32x4 {
@patch class Int32x4 {

should work

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
runtime/lib/typed_data_patch.dart:2031: class _Float64x2 implements Float64x2 {
@patch class Float64x2 {

[Kevin Millikin (Google), 2016-12-19 12:20:50]

Thanks for the review, Florian.

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
File runtime/lib/typed_data_patch.dart (right):

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
runtime/lib/typed_data_patch.dart:1937: class _Float32x4 implements Float32x4 {
On 2016/12/16 22:20:36, Florian Schneider wrote:
> @patch class Float32x4 {
> 
> should work.

That doesn't work without (1) making Float32x4 non-abstract, (2) making all
these methods external, and (3) making a really pretty big change to dart2js
because it does have separate interface and implementation classes in separate
libraries and the implementation classes can't be moved into this library
because they have private members that need to be accessible to the other
library and can't be made public because they would leak into Float32x4.

We could probably make that change, but I'd want to involve the dart2js team.

In any case, it seems out of scope for this change.  As I said before, I think
the right thing to do in the VM's AOT compiler is recognize that there is only
one implementation class and eliminate all overhead due to the interface classes
as an optimization that all code would benefit from.

[Florian Schneider, 2016-12-19 18:52:09]

Lgtm.

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
File runtime/lib/typed_data_patch.dart (right):

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
runtime/lib/typed_data_patch.dart:722: class Int8List {
Why is the 'abstract' modifier missing on all the patch classes? For consistency
I'd make the declaration match the shared SDK. So either remove it there, or add
it here.

https://codereview.chromium.org/2571563005/diff/20001/runtime/lib/typed_data_...
runtime/lib/typed_data_patch.dart:1937: class _Float32x4 implements Float32x4 {
On 2016/12/19 12:20:50, Kevin Millikin (Google) wrote:
> On 2016/12/16 22:20:36, Florian Schneider wrote:
> > @patch class Float32x4 {
> > 
> > should work.
> 
> That doesn't work without (1) making Float32x4 non-abstract, (2) making all
> these methods external, and (3) making a really pretty big change to dart2js
> because it does have separate interface and implementation classes in separate
> libraries and the implementation classes can't be moved into this library
> because they have private members that need to be accessible to the other
> library and can't be made public because they would leak into Float32x4.
> 
> We could probably make that change, but I'd want to involve the dart2js team.
> 
> In any case, it seems out of scope for this change.  As I said before, I think
> the right thing to do in the VM's AOT compiler is recognize that there is only
> one implementation class and eliminate all overhead due to the interface
classes
> as an optimization that all code would benefit from.

That would be a good optimization, but I'd rather not rely on it by default. I
can see why changing dart2js would be out of scope for this CL.

Overall I don't see the point of an abstract class that has a factory with the
same name as the default constructor name. It seems redundant, but that's a
different discussion.

[Kevin Millikin (Google), 2016-12-21 07:35:01]

Description was changed from

==========
Turn the VM's dart:typed_data into a patch

Before, the VM's dart:typed_data was a complete replacement of the SDK's
dart:typed_data implementation instead of a patch.  This is unlike all
the other SDK libraries.  This difference requires special-casing for
dart:typed_data in tools that handle the SDK libraries (e.g., the
Analyzer's patching support, the GN build).

This change makes dart:typed_data back into a patch to the SDK's
implementation.  It reintroduces a distinction between abstract
interface and concrete implementation classes, so there are more
classes.

BUG=
==========

to

==========
Turn the VM's dart:typed_data into a patch

Before, the VM's dart:typed_data was a complete replacement of the SDK's
dart:typed_data implementation instead of a patch.  This is unlike all
the other SDK libraries.  This difference requires special-casing for
dart:typed_data in tools that handle the SDK libraries (e.g., the
Analyzer's patching support, the GN build).

This change makes dart:typed_data back into a patch to the SDK's
implementation.  It reintroduces a distinction between abstract
interface and concrete implementation classes, so there are more
classes.

BUG=
R=fschneider@google.com

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

[Kevin Millikin (Google), 2016-12-21 07:35:02]

Committed patchset #4 (id:60001) manually as
a9b906d319c32525a0600c75c008c92753591d86 (presubmit successful).

[Kevin Millikin (Google), 2016-12-21 14:11:53]

Description was changed from

==========
Turn the VM's dart:typed_data into a patch

Before, the VM's dart:typed_data was a complete replacement of the SDK's
dart:typed_data implementation instead of a patch.  This is unlike all
the other SDK libraries.  This difference requires special-casing for
dart:typed_data in tools that handle the SDK libraries (e.g., the
Analyzer's patching support, the GN build).

This change makes dart:typed_data back into a patch to the SDK's
implementation.  It reintroduces a distinction between abstract
interface and concrete implementation classes, so there are more
classes.

BUG=
R=fschneider@google.com

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

to

==========
Turn the VM's dart:typed_data into a patch

Before, the VM's dart:typed_data was a complete replacement of the SDK's
dart:typed_data implementation instead of a patch.  This is unlike all
the other SDK libraries.  This difference requires special-casing for
dart:typed_data in tools that handle the SDK libraries (e.g., the
Analyzer's patching support, the GN build).

This change makes dart:typed_data back into a patch to the SDK's
implementation.  It reintroduces a distinction between abstract
interface and concrete implementation classes, so there are more
classes.

BUG=
R=fschneider@google.com

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

[Kevin Millikin (Google), 2016-12-21 14:17:00]

This was reverted due to debug build checked mode failures.  The change did not
correctly reintroduce the distinction between the implementation and interface
classes for Float32x4, Int32x4, and Float64x2 everywhere it was needed.

Slava could you take a quick look at the fix, which is patch set #5?

[Vyacheslav Egorov (Google), 2016-12-21 14:44:38]

lgtm

[Kevin Millikin (Google), 2016-12-22 09:57:30]

Description was changed from

==========
Turn the VM's dart:typed_data into a patch

Before, the VM's dart:typed_data was a complete replacement of the SDK's
dart:typed_data implementation instead of a patch.  This is unlike all
the other SDK libraries.  This difference requires special-casing for
dart:typed_data in tools that handle the SDK libraries (e.g., the
Analyzer's patching support, the GN build).

This change makes dart:typed_data back into a patch to the SDK's
implementation.  It reintroduces a distinction between abstract
interface and concrete implementation classes, so there are more
classes.

BUG=
R=fschneider@google.com

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

to

==========
Turn the VM's dart:typed_data into a patch

Before, the VM's dart:typed_data was a complete replacement of the SDK's
dart:typed_data implementation instead of a patch.  This is unlike all
the other SDK libraries.  This difference requires special-casing for
dart:typed_data in tools that handle the SDK libraries (e.g., the
Analyzer's patching support, the GN build).

This change makes dart:typed_data back into a patch to the SDK's
implementation.  It reintroduces a distinction between abstract
interface and concrete implementation classes, so there are more
classes.

BUG=
R=fschneider@google.com, vegorov@google.com

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

[Kevin Millikin (Google), 2016-12-22 09:57:32]

Committed patchset #5 (id:80001) manually as
d9f80a9ac16f7a55ddf1b5a1791b01c93beba8f4 (presubmit successful).