Add --optimize_for_size to tests flag matrix.

Add --optimize_for_size to tests flag matrix.

BUG=None

[rmcilroy, 2013-10-29 12:35:39]

This change requires https://codereview.chromium.org/47743007/ and
https://codereview.chromium.org/47023003/ to land before the tests will run
successfully with the --optimize-for-size flag.

PTAL.

[Sven Panne, 2013-10-29 12:43:31]

NOT LGTM.

https://codereview.chromium.org/49633004/diff/1/tools/run-tests.py
File tools/run-tests.py (right):

https://codereview.chromium.org/49633004/diff/1/tools/run-tests.py#newcode57
tools/run-tests.py:57: ["--optimize-for-size"],
I am not sure if this is the right way to proceed, because it will blow up our
test times significantly. Of course it would be a nice-to-have for each flag,
but we can't afford this due to combinatorial explosion.

Perhaps we can have a runner on our waterfall with this flag enabled instead?

[rmcilroy, 2013-10-29 14:02:54]

https://codereview.chromium.org/49633004/diff/1/tools/run-tests.py
File tools/run-tests.py (right):

https://codereview.chromium.org/49633004/diff/1/tools/run-tests.py#newcode57
tools/run-tests.py:57: ["--optimize-for-size"],
On 2013/10/29 12:43:31, Sven Panne wrote:
> I am not sure if this is the right way to proceed, because it will blow up our
> test times significantly. Of course it would be a nice-to-have for each flag,
> but we can't afford this due to combinatorial explosion.
> 
> Perhaps we can have a runner on our waterfall with this flag enabled instead?

I realize this will increase test time by about 25% - that's why I didn't
introduce it in the original CL - but I think it's important to test with this
flag enabled since it significantly changes generated prologue code (and will
change other generated code as more optimizations are added), and the
run-tests.py tests found some issues which weren't apparent when running the JS
benchmarks.

Having a runner (for each arch) with this flag enabled would be OK, but are
there extra runners available on which we can enable this flag (from what I can
see, there is only a single ARM runner on the V8 waterfall, and I think we want
to test both with and without the flag).  Thoughts?

[Sven Panne, 2013-10-29 14:18:33]

On 2013/10/29 14:02:54, rmcilroy wrote:
> I realize this will increase test time by about 25% - that's why I didn't
> introduce it in the original CL - but I think it's important to test with this
> flag enabled since it significantly changes generated prologue code (and will
> change other generated code as more optimizations are added), and the
> run-tests.py tests found some issues which weren't apparent when running the
JS
> benchmarks.

You can test this locally via e.g.

   make -j32 arm.check TESTFLAGS="--extra-flags --optimize-for-size"

If we kept every new flag for every new feature as a variant in the test suite,
we effectively be dead by now. :-/

> Having a runner (for each arch) with this flag enabled would be OK, but are
> there extra runners available on which we can enable this flag (from what I
can
> see, there is only a single ARM runner on the V8 waterfall, and I think we
want
> to test both with and without the flag).  Thoughts?

What we normally do is that some runners have some flags enabled, others don't.
This way we don't really have a testing *matrix*, but at least some coverage.

I know that this is annoying, and of course everybody likes to get good test
coverage for his own new feature, but it is important to keep test times
manageable. Recently there was a very visible tendency that people obviously
didn't run the test suite at all before committing, and we should actively fight
that tendency...

[Michael Achenbach, 2013-10-29 14:38:02]

I agree to Sven's suggestions. I can set up some builders on the waterfall to
run a special config, once it is ready (just give me a note).

The "arm" builder that runs on real hardware is still quite fast, so I suggest
we split it into two, one with and one without the flag (and optionally one
being a release and the other a debug builder).

Then I just "randomly" select one builder of each other platform to run with
that flag on.

Alternative: When passing in via --extra-flags=--optimize-for-size, every test
variant will have the optimize-for-size flag set. Another option would be to
extend the run-tests script with a new optimize-for-size parameter that, only
when set, adds a variant with just optimize-for-size to the other three
variants. Then we would have +33% on a few selected builders, while also keeping
the old test configurations.

What way to go depends on if you want the cross product of that flag with the
other variants or not.

[rmcilroy, 2013-10-29 15:45:53]

On 2013/10/29 14:38:02, machenbach wrote:
> I agree to Sven's suggestions. I can set up some builders on the waterfall to
> run a special config, once it is ready (just give me a note).
> 
> The "arm" builder that runs on real hardware is still quite fast, so I suggest
> we split it into two, one with and one without the flag (and optionally one
> being a release and the other a debug builder).
> 
> Then I just "randomly" select one builder of each other platform to run with
> that flag on.
> 
> Alternative: When passing in via --extra-flags=--optimize-for-size, every test
> variant will have the optimize-for-size flag set. Another option would be to
> extend the run-tests script with a new optimize-for-size parameter that, only
> when set, adds a variant with just optimize-for-size to the other three
> variants. Then we would have +33% on a few selected builders, while also
keeping
> the old test configurations.
> 
> What way to go depends on if you want the cross product of that flag with the
> other variants or not.

I would be happy with either of these suggestions, I'll chat you tomorrow to
decide on the best approach and work out the fine details of what needs to be
done to get some of the runners running with this flag.

> I know that this is annoying, and of course everybody likes to get good test
> coverage for his own new feature, but it is important to keep test times
> manageable. Recently there was a very visible tendency that people obviously
> didn't run the test suite at all before committing, and we should actively
fight
> that tendency...

This isn't just testing of a feature.  The flag is used in production devices
(low-end Android phones), so I think it is important to have continuous testing
of this since it can interact in unexpected ways with seemingly unrelated
changes in code generation.  I totally agree with you that we should keep the
test runner quick to encourage people to run it before committing changes -
hopefully the suggestions yourself and Michael have made will provide the best
of both worlds.

Thanks.

[Jakob Kummerow, 2013-10-29 15:52:31]

Adding new slave definitions increases cycle time quite considerably when two or
more slaves have to share a VM. Instead, I'd suggest we model this as an
additional step that can be added to a few bots.

Ross, any input on which test suites are useful to run with the flag? mjsunit
probably, maybe test262, mozilla, or benchmarks. cctests, messages, and
preparser probably don't make much sense.

Regarding variants, I'm not a huge fan of the existing variants anyway. It's
probably fine to run the new step with --no-variants
--extra-flags=--optimize-for-size.

(FWIW, I fully agree that adding a fourth variant is not an option. If anything
we should remove one.)

[rmcilroy, 2013-10-29 16:17:01]

On 2013/10/29 15:52:31, Jakob wrote:
> Adding new slave definitions increases cycle time quite considerably when two
or
> more slaves have to share a VM. Instead, I'd suggest we model this as an
> additional step that can be added to a few bots.

SGTM.

> Ross, any input on which test suites are useful to run with the flag? mjsunit
> probably, maybe test262, mozilla, or benchmarks. cctests, messages, and
> preparser probably don't make much sense.

Actually it was cctests which found the previous two issues (bad interactions
when SetFunctionEntryHook was non-null, and when the debugger was active), so I
think it would make sense to include that.  I'm not sure on the other tests, but
I agree messages and preparser are probably less interesting for this step.