[gn] Use external startup data by default

[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
NOTRY=true

Committed: https://crrev.com/ad609ef1d114c85f57c222ebbc8fa1d7c783fc8d
Cr-Commit-Position: refs/heads/master@{#36210}

[Michael Achenbach, 2016-05-12 07:55:27]

Description was changed from

==========
[gn] Use external starup data by default

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

to

==========
[gn] Use external starup data by default

BUG=chromium:474921
LOG=n
==========

[Michael Achenbach, 2016-05-12 07:55:53]

Description was changed from

==========
[gn] Use external starup data by default

BUG=chromium:474921
LOG=n
==========

to

==========
[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
==========

[Michael Achenbach, 2016-05-12 08:06:32]

machenbach@chromium.org changed reviewers:
+ jochen@chromium.org, vogelheim@chromium.org

[Michael Achenbach, 2016-05-12 08:06:33]

PTAL

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn#newcode27
BUILD.gn:27: v8_use_external_startup_data = true
Couldn't find where chromium sets this. Are we maybe not running with external
snapshot at all?

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn#newcode616
BUILD.gn:616: "$target_gen_dir/snapshot.cc",
Isn't it wrong to have this output spec when v8_use_external_startup_data ==
false?

[Michael Achenbach, 2016-05-12 08:07:04]

Description was changed from

==========
[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
==========

to

==========
[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
NOTRY=true
==========

[Michael Achenbach, 2016-05-12 09:24:31]

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn#newcode27
BUILD.gn:27: v8_use_external_startup_data = true
On 2016/05/12 08:06:33, Michael Achenbach wrote:
> Couldn't find where chromium sets this. Are we maybe not running with external
> snapshot at all?

OK. Found it:
https://code.google.com/p/chromium/codesearch#chromium/src/build_overrides/v8...

But we should still set it to true so that we also use the default in
stand-alone.

[vogelheim, 2016-05-12 12:35:10]

lgtm

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn#newcode27
BUILD.gn:27: v8_use_external_startup_data = true
On 2016/05/12 09:24:31, Michael Achenbach wrote:
> On 2016/05/12 08:06:33, Michael Achenbach wrote:
> > Couldn't find where chromium sets this. Are we maybe not running with
external
> > snapshot at all?
> 
> OK. Found it:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build_overrides/v8...
> 
> But we should still set it to true so that we also use the default in
> stand-alone.

Agreed!

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn#newcode616
BUILD.gn:616: "$target_gen_dir/snapshot.cc",
On 2016/05/12 08:06:33, Michael Achenbach wrote:
> Isn't it wrong to have this output spec when v8_use_external_startup_data ==
> false?

Not wrong, but superflous? The way the rule is written (see the args section
below) it always instructs mksnapshot to write the snapshot.cc file, so I guess
it's correct to unconditionally declare it as the output.

I guess the rule _could_ be written differently, so that it will only generate
the file that is used. E.g. by turning the if(v8_use_external_startup_data)...
into if-else.

(Wouldn't make a performance difference, though. The work done my mksnapshot is
mostly the same, regardless out output file.)

[Michael Achenbach, 2016-05-12 14:09:16]

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1971133002/diff/1/BUILD.gn#newcode616
BUILD.gn:616: "$target_gen_dir/snapshot.cc",
On 2016/05/12 12:35:10, vogelheim wrote:
> On 2016/05/12 08:06:33, Michael Achenbach wrote:
> > Isn't it wrong to have this output spec when v8_use_external_startup_data ==
> > false?
> 
> Not wrong, but superflous? The way the rule is written (see the args section
> below) it always instructs mksnapshot to write the snapshot.cc file, so I
guess
> it's correct to unconditionally declare it as the output.
> 
> I guess the rule _could_ be written differently, so that it will only generate
> the file that is used. E.g. by turning the if(v8_use_external_startup_data)...
> into if-else.
> 
> (Wouldn't make a performance difference, though. The work done my mksnapshot
is
> mostly the same, regardless out output file.)

Acknowledged.

[Michael Achenbach, 2016-05-12 14:09:21]

The CQ bit was checked by machenbach@chromium.org

[commit-bot: I haz the power, 2016-05-12 14:09:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971133002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971133002/1

[commit-bot: I haz the power, 2016-05-12 14:10:48]

Description was changed from

==========
[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
NOTRY=true
==========

to

==========
[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
NOTRY=true
==========

[commit-bot: I haz the power, 2016-05-12 14:10:49]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-12 14:12:16]

Description was changed from

==========
[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
NOTRY=true
==========

to

==========
[gn] Use external startup data by default

BUG=chromium:474921
LOG=n
NOTRY=true

Committed: https://crrev.com/ad609ef1d114c85f57c222ebbc8fa1d7c783fc8d
Cr-Commit-Position: refs/heads/master@{#36210}
==========

[commit-bot: I haz the power, 2016-05-12 14:12:17]

Patchset 1 (id:??) landed as
https://crrev.com/ad609ef1d114c85f57c222ebbc8fa1d7c783fc8d
Cr-Commit-Position: refs/heads/master@{#36210}