Add 'flutter_release' patch_sdk mode to patch_sdk

Introduce 'flutter_release' patch_sdk mode to accommodate for difference between Flutter release and non-release target configuration in regards to inclusion of vm_service package.

Flutter non-release should have vm_service package included into platform.dill to handle observatory requests, while release builds should not have it as they are not supposed to have observatory functionality enabled.

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

BUG:dartbug.com/30112
Committed: https://github.com/dart-lang/sdk/commit/6dd692b2399229c8158884455ece325f942bdc88

[aam, 2017-07-08 16:57:45]

Description was changed from

==========
Introduce '--for-release' compiler option to accommodate for difference between
Flutter release and non-release target configuration.

Flutter non-release should have vm_service package included into platform.dill
to handle observatory requests, whlie release builds should not have it as they
are not supposed to have observatory functionality enabled.

BUG:dartbug.com/30112
==========

to

==========
Introduce 'flutter_fasta_release' target to accommodate for difference between
Flutter release and non-release target configuration in regards to inclusion of
vm_service package.

Flutter non-release should have vm_service package included into platform.dill
to handle observatory requests, whlie release builds should not have it as they
are not supposed to have observatory functionality enabled.

BUG:dartbug.com/30112
==========

[aam, 2017-07-08 16:59:21]

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

[aam, 2017-07-08 16:59:22]

PTAL, thanks!

[ahe, 2017-07-10 13:19:02]

lgtm

Please wait for Siggi's comments.

https://codereview.chromium.org/2972323002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:198:
--target=none|vm|vm_fasta|vmcc|vmreify|flutter|flutter_fasta|flutter_fasta_release
Perhaps we should change this to:

  --target=TARGET
    Specify the target configuration, where TARGET is one of 'none', 'vm',
    'vm_fasta', 'vmcc', 'vmreify', 'flutter', 'flutter_fasta', or
    'flutter_fasta_release'.

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
File pkg/kernel/lib/target/flutter.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
pkg/kernel/lib/target/flutter.dart:18: bool includeVMService;
includeVMService -> includeVmService

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
pkg/kernel/lib/target/flutter.dart:18: bool includeVMService;
Make this final.

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
File pkg/kernel/lib/target/flutter_fasta.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
pkg/kernel/lib/target/flutter_fasta.dart:47:
FlutterFastaReleaseTarget(TargetFlags flags) : super(flags) {
super(flags, includeVmService: false);

[Siggi Cherem (dart-lang), 2017-07-10 17:29:58]

Hi Alex,

I mainly have a design question: I thought SivaC removed the need for having
vmservice within platform.dill and the idea was to have a separate .dill file
for it instead. That's how it currently works for the standalone VM, is that
model not possible for the flutter engine?

I made a note below in patch_sdk.dart where we do that.

If that approach is possible, I honestly would prefer to go down that route.
We'd then not need another target for flutter-release in kernel and fasta.

https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:139: var vmserviceName = 'vmservice_io';
I thought instead we would compile vmservice_sky here.

[sivachandra, 2017-07-10 18:22:48]

On 2017/07/10 17:29:58, Siggi Cherem (dart-lang) wrote:
> Hi Alex,
> 
> I mainly have a design question: I thought SivaC removed the need for having
> vmservice within platform.dill and the idea was to have a separate .dill file
> for it instead. That's how it currently works for the standalone VM, is that
> model not possible for the flutter engine?
> 
> I made a note below in patch_sdk.dart where we do that.
> 
> If that approach is possible, I honestly would prefer to go down that route.
> We'd then not need another target for flutter-release in kernel and fasta.
> 
> https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart
> File tools/patch_sdk.dart (right):
> 
>
https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart#ne...
> tools/patch_sdk.dart:139: var vmserviceName = 'vmservice_io';
> I thought instead we would compile vmservice_sky here.

I also have few questions around the higher level design. The description of
this change points out the we need vmservice only for debug builds.

asiva - Are we sensitive to the flutter engine binary size for debug builds as
well? If vmservice is not required for release builds, could we just link it in
with the dart executable/flutter engine? Siggi landed a change over the weekend
which removed sources from outline.dill so the vmservice_io.dill file can now be
~600K.

[Siggi Cherem (dart-lang), 2017-07-10 18:41:48]

On 2017/07/10 18:22:48, sivachandra wrote:
> On 2017/07/10 17:29:58, Siggi Cherem (dart-lang) wrote:
> > Hi Alex,
> > 
> > I mainly have a design question: I thought SivaC removed the need for having
> > vmservice within platform.dill and the idea was to have a separate .dill
file
> > for it instead. That's how it currently works for the standalone VM, is that
> > model not possible for the flutter engine?
> > 
> > I made a note below in patch_sdk.dart where we do that.
> > 
> > If that approach is possible, I honestly would prefer to go down that route.
> > We'd then not need another target for flutter-release in kernel and fasta.
> > 
> > https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart
> > File tools/patch_sdk.dart (right):
> > 
> >
>
https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart#ne...
> > tools/patch_sdk.dart:139: var vmserviceName = 'vmservice_io';
> > I thought instead we would compile vmservice_sky here.
> 
> I also have few questions around the higher level design. The description of
> this change points out the we need vmservice only for debug builds.
> 
> asiva - Are we sensitive to the flutter engine binary size for debug builds as
> well? If vmservice is not required for release builds, could we just link it
in
> with the dart executable/flutter engine? Siggi landed a change over the
weekend
> which removed sources from outline.dill so the vmservice_io.dill file can now
be
> ~600K.

BTW - I hit two issues in debug-mode with my CL and I had to partially revert
(we are still not using outline.dill). I believe the issues can be fixed, so we
should be able to get back to the smaller size. See
https://github.com/dart-lang/sdk/issues/30111

[aam, 2017-07-10 21:51:23]

> I mainly have a design question: I thought SivaC removed the need for having
> vmservice within platform.dill and the idea was to have a separate .dill file
> for it instead. That's how it currently works for the standalone VM, is that
> model not possible for the flutter engine?

  My understanding is that that change broke frontend-based gen_snapshot for
release/profiles build modes. Those are the flows I'm trying to fix now.

> I made a note below in patch_sdk.dart where we do that.
> 
> If that approach is possible, I honestly would prefer to go down that route.
> We'd then not need another target for flutter-release in kernel and fasta.
  
  Sure, I understand. 
  When I put this CL together the thinking was the following. When we compile
main.dart [flutter] script, contents of platform.dill used for compilation gets
added into resultant main.dart.dill. For (profile and release modes) that single
main.dart.dill is then fed into gen_snapshot. (For profile mode) gen_snapshot
resolves references to dart:_vmservice, dart:vmservice_sky libraries.
gen_snapshot does not load additional platform.dill/vmservice.dills - instead it
loads everything from main.dart.dill file. So main.dart.dill should have
vmservice libraries for gen_snapshot to find and resolve them. 
  "debug/developer" mode of Flutter build doesn't use gen_snapshot - instead it
sends all dill files to the device, where they are loaded by VM, generally
speaking can use platform(_release).dill+vmservice.dill combo.

  Before I move forward with this CL I need to confirm that gen_snapshot in
release mode can really work with platform.dill without vmservice(currently it
actually can't - vmservice library has to be present in "release" as well as in
"profile" modes).

[aam, 2017-07-10 22:11:37]

Thank you folks!

https://codereview.chromium.org/2972323002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:198:
--target=none|vm|vm_fasta|vmcc|vmreify|flutter|flutter_fasta|flutter_fasta_release
On 2017/07/10 13:19:02, ahe wrote:
> Perhaps we should change this to:
> 
>   --target=TARGET
>     Specify the target configuration, where TARGET is one of 'none', 'vm',
>     'vm_fasta', 'vmcc', 'vmreify', 'flutter', 'flutter_fasta', or
>     'flutter_fasta_release'.

Done.

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
File pkg/kernel/lib/target/flutter.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
pkg/kernel/lib/target/flutter.dart:18: bool includeVMService;
On 2017/07/10 13:19:02, ahe wrote:
> includeVMService -> includeVmService

Done.

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
pkg/kernel/lib/target/flutter.dart:18: bool includeVMService;
On 2017/07/10 13:19:02, ahe wrote:
> Make this final.

I can't do that because I can't use "super(flags, includeVmService: false);"
form in FlutterFastaReleaseTarget class.

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
File pkg/kernel/lib/target/flutter_fasta.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/pkg/kernel/lib/target/f...
pkg/kernel/lib/target/flutter_fasta.dart:47:
FlutterFastaReleaseTarget(TargetFlags flags) : super(flags) {
On 2017/07/10 13:19:02, ahe wrote:
> super(flags, includeVmService: false);

I tried doing something like that but got stuck with
===
'package:kernel/target/flutter_fasta.dart': error: line 49 pos 50: invalid
arguments passed to super class constructor 'FlutterFastaTarget.': 1 named
passed, at most 0 expected
  FlutterFastaReleaseTarget(TargetFlags flags) : super(flags, includeVmService:
false);
                                                 ^
===
therefore had to fallback to current form.
Am I doing something wrong?

https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:139: var vmserviceName = 'vmservice_io';
On 2017/07/10 17:29:57, Siggi Cherem (dart-lang) wrote:
> I thought instead we would compile vmservice_sky here.

Yeah, something like that make sense, yet somehow for Flutter debug mode it
works without vmservice_sky.dill. I need to look further into this, sync up with
SivaB on that.

[Siggi Cherem (dart-lang), 2017-07-10 22:34:36]

On 2017/07/10 21:51:23, aam wrote:
> > I mainly have a design question: I thought SivaC removed the need for having
> > vmservice within platform.dill and the idea was to have a separate .dill
file
> > for it instead. That's how it currently works for the standalone VM, is that
> > model not possible for the flutter engine?
> 
>   My understanding is that that change broke frontend-based gen_snapshot for
> release/profiles build modes. Those are the flows I'm trying to fix now.
> 
> > I made a note below in patch_sdk.dart where we do that.
> > 
> > If that approach is possible, I honestly would prefer to go down that route.
> > We'd then not need another target for flutter-release in kernel and fasta.
>   
>   Sure, I understand. 
>   When I put this CL together the thinking was the following. When we compile
> main.dart [flutter] script, contents of platform.dill used for compilation
gets
> added into resultant main.dart.dill. For (profile and release modes) that
single
> main.dart.dill is then fed into gen_snapshot. (For profile mode) gen_snapshot
> resolves references to dart:_vmservice, dart:vmservice_sky libraries.
> gen_snapshot does not load additional platform.dill/vmservice.dills - instead
it
> loads everything from main.dart.dill file. So main.dart.dill should have
> vmservice libraries for gen_snapshot to find and resolve them. 
>   "debug/developer" mode of Flutter build doesn't use gen_snapshot - instead
it
> sends all dill files to the device, where they are loaded by VM, generally
> speaking can use platform(_release).dill+vmservice.dill combo.
> 
>   Before I move forward with this CL I need to confirm that gen_snapshot in
> release mode can really work with platform.dill without vmservice(currently it
> actually can't - vmservice library has to be present in "release" as well as
in
> "profile" modes).

I see, thanks for the context.

A couple notes:

 - Just this weekend I've switched patch_sdk to use the public front_end APIs.
The new API give you a lot more flexibility without having to to define new
targets. In particular, you can list explicitly any `dart:*` library you want to
include in platform.dill (see
https://github.com/dart-lang/sdk/blob/master/tools/patch_sdk.dart#L220). If you
need to continue to add vmservice to the platform file, I believe you can simply
add the vmservice* libraries directly there.

 - Today we include `platform.dill` with the app, but we are trying to change it
to only include `outline.dill` or even a tree-shaken version of that. So I hope
gen_snapshot is not relying on this old behavior (if it is, can it move away
from it?)

[Siggi Cherem (dart-lang), 2017-07-10 22:39:43]

On 2017/07/10 22:34:36, Siggi Cherem (dart-lang) wrote:
> On 2017/07/10 21:51:23, aam wrote:
> > > I mainly have a design question: I thought SivaC removed the need for
having
> > > vmservice within platform.dill and the idea was to have a separate .dill
> file
> > > for it instead. That's how it currently works for the standalone VM, is
that
> > > model not possible for the flutter engine?
> > 
> >   My understanding is that that change broke frontend-based gen_snapshot for
> > release/profiles build modes. Those are the flows I'm trying to fix now.
> > 
> > > I made a note below in patch_sdk.dart where we do that.
> > > 
> > > If that approach is possible, I honestly would prefer to go down that
route.
> > > We'd then not need another target for flutter-release in kernel and fasta.
> >   
> >   Sure, I understand. 
> >   When I put this CL together the thinking was the following. When we
compile
> > main.dart [flutter] script, contents of platform.dill used for compilation
> gets
> > added into resultant main.dart.dill. For (profile and release modes) that
> single
> > main.dart.dill is then fed into gen_snapshot. (For profile mode)
gen_snapshot
> > resolves references to dart:_vmservice, dart:vmservice_sky libraries.
> > gen_snapshot does not load additional platform.dill/vmservice.dills -
instead
> it
> > loads everything from main.dart.dill file. So main.dart.dill should have
> > vmservice libraries for gen_snapshot to find and resolve them. 
> >   "debug/developer" mode of Flutter build doesn't use gen_snapshot - instead
> it
> > sends all dill files to the device, where they are loaded by VM, generally
> > speaking can use platform(_release).dill+vmservice.dill combo.
> > 
> >   Before I move forward with this CL I need to confirm that gen_snapshot in
> > release mode can really work with platform.dill without vmservice(currently
it
> > actually can't - vmservice library has to be present in "release" as well as
> in
> > "profile" modes).
> 
> I see, thanks for the context.
> 
> A couple notes:
> 
>  - Just this weekend I've switched patch_sdk to use the public front_end APIs.
> The new API give you a lot more flexibility without having to to define new
> targets. In particular, you can list explicitly any `dart:*` library you want
to
> include in platform.dill (see
> https://github.com/dart-lang/sdk/blob/master/tools/patch_sdk.dart#L220). If
you
> need to continue to add vmservice to the platform file, I believe you can
simply
> add the vmservice* libraries directly there.

Sorry for not thinking of this earlier - but if we have to add vmservice, let's
use this approach. I think that make all changes local to patch_sdk.dart.

> 
>  - Today we include `platform.dill` with the app, but we are trying to change
it
> to only include `outline.dill` or even a tree-shaken version of that. So I
hope
> gen_snapshot is not relying on this old behavior (if it is, can it move away
> from it?)

[aam, 2017-07-12 01:54:53]

On 2017/07/10 22:39:43, Siggi Cherem (dart-lang) wrote:
> On 2017/07/10 22:34:36, Siggi Cherem (dart-lang) wrote:
> > On 2017/07/10 21:51:23, aam wrote:
> > > > I mainly have a design question: I thought SivaC removed the need for
> having
> > > > vmservice within platform.dill and the idea was to have a separate .dill
> > file
> > > > for it instead. That's how it currently works for the standalone VM, is
> that
> > > > model not possible for the flutter engine?
> > > 
> > >   My understanding is that that change broke frontend-based gen_snapshot
for
> > > release/profiles build modes. Those are the flows I'm trying to fix now.
> > > 
> > > > I made a note below in patch_sdk.dart where we do that.
> > > > 
> > > > If that approach is possible, I honestly would prefer to go down that
> route.
> > > > We'd then not need another target for flutter-release in kernel and
fasta.
> > >   
> > >   Sure, I understand. 
> > >   When I put this CL together the thinking was the following. When we
> compile
> > > main.dart [flutter] script, contents of platform.dill used for compilation
> > gets
> > > added into resultant main.dart.dill. For (profile and release modes) that
> > single
> > > main.dart.dill is then fed into gen_snapshot. (For profile mode)
> gen_snapshot
> > > resolves references to dart:_vmservice, dart:vmservice_sky libraries.
> > > gen_snapshot does not load additional platform.dill/vmservice.dills -
> instead
> > it
> > > loads everything from main.dart.dill file. So main.dart.dill should have
> > > vmservice libraries for gen_snapshot to find and resolve them. 
> > >   "debug/developer" mode of Flutter build doesn't use gen_snapshot -
instead
> > it
> > > sends all dill files to the device, where they are loaded by VM, generally
> > > speaking can use platform(_release).dill+vmservice.dill combo.
> > > 
> > >   Before I move forward with this CL I need to confirm that gen_snapshot
in
> > > release mode can really work with platform.dill without
vmservice(currently
> it
> > > actually can't - vmservice library has to be present in "release" as well
as
> > in
> > > "profile" modes).
> > 
> > I see, thanks for the context.
> > 
> > A couple notes:
> > 
> >  - Just this weekend I've switched patch_sdk to use the public front_end
APIs.
> > The new API give you a lot more flexibility without having to to define new
> > targets. In particular, you can list explicitly any `dart:*` library you
want
> to
> > include in platform.dill (see
> > https://github.com/dart-lang/sdk/blob/master/tools/patch_sdk.dart#L220). If
> you
> > need to continue to add vmservice to the platform file, I believe you can
> simply
> > add the vmservice* libraries directly there.
> 
> Sorry for not thinking of this earlier - but if we have to add vmservice,
let's
> use this approach. I think that make all changes local to patch_sdk.dart.

No worries! Done, thanks.

[siva, 2017-07-12 15:47:14]

On 2017/07/10 18:22:48, sivachandra wrote:
> On 2017/07/10 17:29:58, Siggi Cherem (dart-lang) wrote:
> > Hi Alex,
> > 
> > I mainly have a design question: I thought SivaC removed the need for having
> > vmservice within platform.dill and the idea was to have a separate .dill
file
> > for it instead. That's how it currently works for the standalone VM, is that
> > model not possible for the flutter engine?
> > 
> > I made a note below in patch_sdk.dart where we do that.
> > 
> > If that approach is possible, I honestly would prefer to go down that route.
> > We'd then not need another target for flutter-release in kernel and fasta.
> > 
> > https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart
> > File tools/patch_sdk.dart (right):
> > 
> >
>
https://codereview.chromium.org/2972323002/diff/20001/tools/patch_sdk.dart#ne...
> > tools/patch_sdk.dart:139: var vmserviceName = 'vmservice_io';
> > I thought instead we would compile vmservice_sky here.
> 
> I also have few questions around the higher level design. The description of
> this change points out the we need vmservice only for debug builds.
> 
> asiva - Are we sensitive to the flutter engine binary size for debug builds as
> well? If vmservice is not required for release builds, could we just link it
in
> with the dart executable/flutter engine? Siggi landed a change over the
weekend
> which removed sources from outline.dill so the vmservice_io.dill file can now
be
> ~600K.

vmservice is also needed for the 'profile' build not just the 'debug' build. The
profile build is an AOT application which includes vmserice, currently we don't
have a way of creating two AOT apps one for the service isolate and another for
the regular user app so we bundle the vmservice part also into the regular user
app and both the main isolate and service isolate start with this app image.

The binary size is always a concern for us, debug builds require pushing an
engine to the device when we do 'flutter run' and it becomes part of the initial
application run process.

If we have a working version of the 600k vmservice dill file then yes it can be
linked into the executable for 'debug' builds but we will still have to account
for the 'profile' build

[Siggi Cherem (dart-lang), 2017-07-12 18:06:18]

lgtm, thanks for making the updates!

https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:359: if (!forFlutterRelease) {
is the intent to eventually get rid of this? I mean, to make the flutter-engine
support loading vmservice from a separate .dill file in the profile/debug cases?

If so - let's add a TODO here about it.

[aam, 2017-07-13 16:30:56]

https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:359: if (!forFlutterRelease) {
On 2017/07/12 18:06:18, Siggi Cherem (dart-lang) wrote:
> is the intent to eventually get rid of this? I mean, to make the
flutter-engine
> support loading vmservice from a separate .dill file in the profile/debug
cases?
> 
> If so - let's add a TODO here about it.

If I understand correctly, we won't be able to load more than two
dills(platform.dill+script.dill) into vm isolate. So having three
dills(platform.dill+script.dill+vmservice.dill) won't work when we need to load
all of them into vm isolate to produce single aot snapshot with app script and
vmservice in it[which we do for profile build].

[Siggi Cherem (dart-lang), 2017-07-13 16:47:02]

On 2017/07/13 16:30:56, aam wrote:
> https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart
> File tools/patch_sdk.dart (right):
> 
>
https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart#ne...
> tools/patch_sdk.dart:359: if (!forFlutterRelease) {
> On 2017/07/12 18:06:18, Siggi Cherem (dart-lang) wrote:
> > is the intent to eventually get rid of this? I mean, to make the
> flutter-engine
> > support loading vmservice from a separate .dill file in the profile/debug
> cases?
> > 
> > If so - let's add a TODO here about it.
> 
> If I understand correctly, we won't be able to load more than two
> dills(platform.dill+script.dill) into vm isolate. So having three
> dills(platform.dill+script.dill+vmservice.dill) won't work when we need to
load
> all of them into vm isolate to produce single aot snapshot with app script and
> vmservice in it[which we do for profile build].

interesting, I guess an alternative is to embed vmservice with the script when
generating the aot snapshot

[aam, 2017-07-13 16:53:26]

On 2017/07/13 16:47:02, Siggi Cherem (dart-lang) wrote:
> On 2017/07/13 16:30:56, aam wrote:
> > https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart
> > File tools/patch_sdk.dart (right):
> > 
> >
>
https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart#ne...
> > tools/patch_sdk.dart:359: if (!forFlutterRelease) {
> > On 2017/07/12 18:06:18, Siggi Cherem (dart-lang) wrote:
> > > is the intent to eventually get rid of this? I mean, to make the
> > flutter-engine
> > > support loading vmservice from a separate .dill file in the profile/debug
> > cases?
> > > 
> > > If so - let's add a TODO here about it.
> > 
> > If I understand correctly, we won't be able to load more than two
> > dills(platform.dill+script.dill) into vm isolate. So having three
> > dills(platform.dill+script.dill+vmservice.dill) won't work when we need to
> load
> > all of them into vm isolate to produce single aot snapshot with app script
and
> > vmservice in it[which we do for profile build].
> 
> interesting, I guess an alternative is to embed vmservice with the script when
> generating the aot snapshot

Do you mean to embed vmservice library into script.dill that is fed into
gen_snapshot? This is what is currently happening because when we produce
script.dill we use platform.dill with vmservice library in it. Not sure - is it
possible to produce script.dill with libraries coming from platform.dill and
vmservice.dill?

[sivachandra, 2017-07-13 17:44:06]

On 2017/07/13 16:30:56, aam wrote:
> https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart
> File tools/patch_sdk.dart (right):
> 
>
https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart#ne...
> tools/patch_sdk.dart:359: if (!forFlutterRelease) {
> On 2017/07/12 18:06:18, Siggi Cherem (dart-lang) wrote:
> > is the intent to eventually get rid of this? I mean, to make the
> flutter-engine
> > support loading vmservice from a separate .dill file in the profile/debug
> cases?
> > 
> > If so - let's add a TODO here about it.
> 
> If I understand correctly, we won't be able to load more than two
> dills(platform.dill+script.dill) into vm isolate. So having three
> dills(platform.dill+script.dill+vmservice.dill) won't work when we need to
load
> all of them into vm isolate to produce single aot snapshot with app script and
> vmservice in it[which we do for profile build].

IIUC, one will not be able to load platform.dill + script.dill + <another>.dill
if <another>.dill does not have a main function. However, vmservice_io.dill has
a main function so this should be possible. AFAIU, this is already being done in
the dartkp configuration.

[Siggi Cherem (dart-lang), 2017-07-13 18:13:15]

On 2017/07/13 17:44:06, sivachandra wrote:
> On 2017/07/13 16:30:56, aam wrote:
> > https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart
> > File tools/patch_sdk.dart (right):
> > 
> >
>
https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart#ne...
> > tools/patch_sdk.dart:359: if (!forFlutterRelease) {
> > On 2017/07/12 18:06:18, Siggi Cherem (dart-lang) wrote:
> > > is the intent to eventually get rid of this? I mean, to make the
> > flutter-engine
> > > support loading vmservice from a separate .dill file in the profile/debug
> > cases?
> > > 
> > > If so - let's add a TODO here about it.
> > 
> > If I understand correctly, we won't be able to load more than two
> > dills(platform.dill+script.dill) into vm isolate. So having three
> > dills(platform.dill+script.dill+vmservice.dill) won't work when we need to
> load
> > all of them into vm isolate to produce single aot snapshot with app script
and
> > vmservice in it[which we do for profile build].
> 
> IIUC, one will not be able to load platform.dill + script.dill +
<another>.dill
> if <another>.dill does not have a main function. However, vmservice_io.dill
has
> a main function so this should be possible. AFAIU, this is already being done
in
> the dartkp configuration.

It is also possible to do it in a different order: platform + vmservice +
script, for example by doing this:
var options = new CompilerOptions()                                            
     ..target = new FlutterFastaTarget(new TargetFlags())                       
      
     ..packagesFileUri = Uri.base.resolve('.packages')                          
 
     ..compileSdk = true                                                        
 
     ..linkedDependencies = [UriToPlatformDill, UriToVmServiceDill])]           
     
     ..onError = errorHandler; 
var scriptDill = kernelForProgram(scriptEntryUri, options);

[aam, 2017-07-13 19:25:30]

On 2017/07/13 18:13:15, Siggi Cherem (dart-lang) wrote:
> On 2017/07/13 17:44:06, sivachandra wrote:
> > On 2017/07/13 16:30:56, aam wrote:
> > > https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart
> > > File tools/patch_sdk.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2972323002/diff/60001/tools/patch_sdk.dart#ne...
> > > tools/patch_sdk.dart:359: if (!forFlutterRelease) {
> > > On 2017/07/12 18:06:18, Siggi Cherem (dart-lang) wrote:
> > > > is the intent to eventually get rid of this? I mean, to make the
> > > flutter-engine
> > > > support loading vmservice from a separate .dill file in the
profile/debug
> > > cases?
> > > > 
> > > > If so - let's add a TODO here about it.
> > > 
> > > If I understand correctly, we won't be able to load more than two
> > > dills(platform.dill+script.dill) into vm isolate. So having three
> > > dills(platform.dill+script.dill+vmservice.dill) won't work when we need to
> > load
> > > all of them into vm isolate to produce single aot snapshot with app script
> and
> > > vmservice in it[which we do for profile build].
> > 
> > IIUC, one will not be able to load platform.dill + script.dill +
> <another>.dill
> > if <another>.dill does not have a main function. However, vmservice_io.dill
> has
> > a main function so this should be possible. AFAIU, this is already being
done
> in
> > the dartkp configuration.
> 
> It is also possible to do it in a different order: platform + vmservice +
> script, for example by doing this:
> var options = new CompilerOptions()                                           

>      ..target = new FlutterFastaTarget(new TargetFlags())                     
 
>       
>      ..packagesFileUri = Uri.base.resolve('.packages')                        
 
>  
>      ..compileSdk = true                                                      
 
>  
>      ..linkedDependencies = [UriToPlatformDill, UriToVmServiceDill])]         
 
>      
>      ..onError = errorHandler; 
> var scriptDill = kernelForProgram(scriptEntryUri, options);

Ah, nice! Will explore that. Added TODO for that.

[aam, 2017-07-13 19:38:12]

Description was changed from

==========
Introduce 'flutter_fasta_release' target to accommodate for difference between
Flutter release and non-release target configuration in regards to inclusion of
vm_service package.

Flutter non-release should have vm_service package included into platform.dill
to handle observatory requests, whlie release builds should not have it as they
are not supposed to have observatory functionality enabled.

BUG:dartbug.com/30112
==========

to

==========
Introduce 'flutter_release' patch_sdk mode to accommodate for difference between
Flutter release and non-release target configuration in regards to inclusion of
vm_service package.

Flutter non-release should have vm_service package included into platform.dill
to handle observatory requests, while release builds should not have it as they
are not supposed to have observatory functionality enabled.

BUG:dartbug.com/30112
==========

[aam, 2017-07-13 20:39:09]

Description was changed from

==========
Introduce 'flutter_release' patch_sdk mode to accommodate for difference between
Flutter release and non-release target configuration in regards to inclusion of
vm_service package.

Flutter non-release should have vm_service package included into platform.dill
to handle observatory requests, while release builds should not have it as they
are not supposed to have observatory functionality enabled.

BUG:dartbug.com/30112
==========

to

==========
Introduce 'flutter_release' patch_sdk mode to accommodate for difference between
Flutter release and non-release target configuration in regards to inclusion of
vm_service package.

Flutter non-release should have vm_service package included into platform.dill
to handle observatory requests, while release builds should not have it as they
are not supposed to have observatory functionality enabled.

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

BUG:dartbug.com/30112
Committed:
https://github.com/dart-lang/sdk/commit/6dd692b2399229c8158884455ece325f942bdc88
==========

[aam, 2017-07-13 20:39:11]

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