1388785 - Better testing for wasm tiering

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Lars T Hansen [:lth]]

(I'm filing this as a followup so as not to delay tiering further, but this is not a large amount of work.)

In existing code, one can tag test cases with "test-also-wasm-baseline", which causes the test case to be run both with and without --wasm-always-baseline.

When tiering (bug 1277562) lands, the situation changes so that one can tag test cases with "test-also-no-wasm-baseline" instead; doing so causes the test case to be run both with and without --no-wasm-baseline.

Unfortunately that change leads to different test coverage than before.  In the past, the test would be run both baseline-compiled and ion-compiled.  With tiering, without the flag, the test is run baseline-compiled until the ion code is ready and is patchd in, and then ion-compiled; with the flag, it is run ion-compiled always.  There's no way to truly guarantee that only baseline-compiled code is run.

Tiering introduces a new shell flag, --no-wasm-ion, which can be used to work around that, and the testing system should be expanded to make use of it.  For example, we could have "test-also-no-wasm-ion" as an additional flag, and/or we could have the abbreviation "test-wasm-tiering" imply the combination of the two, for a total of three runs: without any flags, with --no-wasm-ion, and with --no-wasm-baseline.

Comment 1

[Lars T Hansen [:lth]]

Furthermore:

In bug 1277562 there are some patches (designated 16c, 16d, and 16e) that are not landing in the initial wave because there's no agreement on them yet: they implement command line switches to the JS shell, environment variables (useful with the browser), and various ways of manipulating tiering from the command line and from JS.  This functionality has been helpful in finding many bugs in the tiering code and should land in some form or other.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1388785-shell-debugging-switches.patch

Patch 16c from bug 1277652.  For Luke's comments to this patch, see https://bugzilla.mozilla.org/show_bug.cgi?id=1277562#c122.

Note that the disableTier2 flag tests different code than disabling the ion compiler.  When the ion compiler is disabled, the compilation mode is determined early to be baseline-only.  When tier2 is disabled through this switch, the ion compilation is forced to fail after having been scheduled (at least that's the theory).

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1388785-testing-functions.patch

Patch 16d from bug 1277562: implement JS testing functions for this functionality, and some test cases.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1388785-environment-variables.patch

Patch 16e from bug 1277562: implement some environment variables that can be used from the command line to affect behavior also in the browser.

Comment 5

[Lars T Hansen [:lth]]

See also discussion starting at bug 1404683 comment 16.  We *must* have "test-also-no-wasm-ion" or "test-wasm-tiering" as discussed in comment 0 above, otherwise rabaldr is likely not to be tested at all since the tiering rules will take us directly to ion for almost every program.  This should happen soon -- right now we're flying blind.

In general we should also consider whether the standard test switches (the combinations generated by --tbpl) are desirable for wasm compilation; I expect they probably are not and that it's a waste of time to run wasm tests with all these combinations.  (Probably depends on how much JS there is in the test.)  For one thing, standard Ion / Baseline switches do not affect Baldr / Rabaldr, and we have our own rules for background compilation and so on.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1388785-force-wasm-baseline.patch

This introduces a directive, test-also-no-wasm-ion, and adds that to the directives.txt files so that we get some test coverage of rabaldr, which now has few operative regression tests.

It is quite possible that this directive will disappear and that we'll have some tiering directive instead; I just don't want to go without test coverage until then.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8916940 [details] [diff] [review]
bug1388785-force-wasm-baseline.patch

Review of attachment 8916940 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 8

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6adeac346cc0aea40f440bf725353834416621
Bug 1388785 - also force rabaldr to be tested. r=bbouvier

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fa6adeac346c

Comment 10

[Luke Wagner [:luke]]

Attachment: test-wasm-tiering

So thinking about better testing that would've caught bug 1406041, how about this:
 1. add a new shell flag --test-wasm-tiering that:
   a. forces tiering except where disabled by debugging or !BaselineCanCompile()
   b. blocks on tier2 completion upon instantiation of a Module
 2. add a new jit-test directive test-also-wasm-tiering that sets this flag

Comment 11

[Luke Wagner [:luke]]

Attachment: change-wasm-defaults

Unrelated, but I noticed the default ContextOptions disable wasm/wasm-ion/wasm-baseline.  This patch changes the default to match the shipping default.

Comment 12

[Luke Wagner [:luke]]

Attachment: rm-wasm-always-bce

I also noticed this testing mode which has been around a year.  I'm open to hear other opinions, but it seems to me it's not providing much value:
 - it simply controls whether we run an optimization pass in Ion (no effect on Baseline nor later in Cretonne)
 - I haven't seen it catch anything
 - I expect we get more thorough OOB testing via ASan/Valgrind

Comment 13

[Luke Wagner [:luke]]

Attachment: test-wasm-tiering

(Oops, forgot to qref.)

Comment 14

[Lars T Hansen [:lth]]

Comment on attachment 8919939 [details] [diff] [review]
change-wasm-defaults

Review of attachment 8919939 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine, and entails no functional change anywhere as far as I can tell, but removes a footgun.

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 8919940 [details] [diff] [review]
rm-wasm-always-bce

Review of attachment 8919940 [details] [diff] [review]:
-----------------------------------------------------------------

It seems that this switch may have had a different function before, wherein it affected code generation?  Anyway I agree this provides very little value now, if any.

Comment 16

[Lars T Hansen [:lth]]

Comment on attachment 8919942 [details] [diff] [review]
test-wasm-tiering

Review of attachment 8919942 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good testing mode but I think we'll need more modes, so the '--test-wasm-tiering' name is too broad imo.  How about '--test-wasm-await-tier2' (yeah, clunky; open to other suggestions) instead?

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

wasm-always-bce was indeed more useful before: it was unconditionally generating bounds checks for all the memory accesses, potentially revealing errors with:

- signal handlers handling accesses they shouldn't have handled on WASM_HUGE platforms (asm.js). Not sure it ever happened in practice...
- removed bounds checks that shouldn't have been, with wasm BCE on !WASM_HUGE platforms. There have been a few instances of this, and that was why the test directive had been introduced.

It seems it has been tested enough, and since this code hasn't changed for a while, it makes sense to remove it to lower resource consumption on pushes. If we were to get back to WasmBCE.cpp, it'd be nice to have it again, though.

Comment 18

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #16)
> Comment on attachment 8919942 [details] [diff] [review]
> test-wasm-tiering
> 
> Review of attachment 8919942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a good testing mode but I think we'll need more modes, so the
> '--test-wasm-tiering' name is too broad imo.  How about
> '--test-wasm-await-tier2' (yeah, clunky; open to other suggestions) instead?

BTW the 'test-also-wasm-tiering' directive seems like the right thing with the right name, and once we have more testing switches / modes the directive can expand into these multiple modes.  It is just the propagation of that name into the shell and engine I think ought to have a narrower name.

Comment 19

[Luke Wagner [:luke]]

Ok, great points.  I'll keep test-also-wasm-tiering and change shell flag to --test-wasm-await-tier2.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b84fae5dfa32
Change wasm flags' default states in ContextOptions (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c637d9a5317a
Remove --wasm-check-bce flag (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1dc87a94262
Add --test-wasm-await-tier2 and use it in jit-tests (r=lth)

Comment 21

[Lars T Hansen [:lth]]

Re-adding leave-open, I think we need more here.

Comment 22

[Lars T Hansen [:lth]]

Ah, my bad, I see you assigned this to yourself.  Your call, then.

Comment 23

[Luke Wagner [:luke]]

Ah, I didn't know any of us had any imminent plans to add further testing here so I thought best to close out this bug and file new bugs for future work.  One can definitely imagine bugs that only happened when the tier2 transition happens after or during some amount of tier1 execution, but I couldn't think of a way to test this in an orthogonal manner like --test-wasm-await-tier2 does.

Comment 24

[Lars T Hansen [:lth]]

I agree it's difficult to say what more we want.  The WASM_DELAY_TIER2=n switch to just hold up the start of tier2 compilation for n ms did find some problems, as I remember it, but I'm not sure what its value would be now.

In the past we've discussed various chaos modes.  One could imagine a switch that forces the patching to patch only every other entry or similar.  This would then (probabilistically) test that patched and unpatched code intercall properly.  Not clear to me what the value is, once we've established this works.

We can revisit if/when we get around to patching imports and exports :)

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b84fae5dfa32
https://hg.mozilla.org/mozilla-central/rev/c637d9a5317a
https://hg.mozilla.org/mozilla-central/rev/b1dc87a94262

Comment 26

[Lars T Hansen [:lth]]

Comment on attachment 8919942 [details] [diff] [review]
test-wasm-tiering

(Clearing r? because patch with requested fixes landed already.)

Comment 27

[Luke Wagner [:luke]]

Oh my, I had thought that patch already had r+; I must have confused that with another r+. Sorry about that, didn't mean to land preemptively.