1346068 - Implement $262.agent for test262 harness

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Lars T Hansen [:lth]]

So there was this thing in the initial commit that was called harness/agents_spidermonkey.js.  We should be able to use that.

Once developed, where should $262.agent go?  Does it go into the test262 repo or do we put it into m-c?

Comment 2

[Lars T Hansen [:lth]]

Attachment: agent_spidermonkey.js

This is harness/agents_spidermonkey.js from the tip of my SAB branch off test262, so it is probably the most recent thing I have.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Lars T Hansen [:lth] from comment #1)
> So there was this thing in the initial commit that was called
> harness/agents_spidermonkey.js.  We should be able to use that.
> 
> Once developed, where should $262.agent go?  Does it go into the test262
> repo or do we put it into m-c?

We hook up the host-defined functions via js/src/tests/test262-host.js. You could add it there, then uncomment out the built-ins/Atomics skip lines in js/src/tests/jstests.list.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1346068-test262-agent.patch

I simply imported and tightened up the code, manually patched test262/shell.js, and removed the comments as you indicated.  The Atomics and SharedArrayBuffer test directories pass locally.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8847083 [details] [diff] [review]
bug1346068-test262-agent.patch

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

Could you make sure that nothing breaks if the global doesn't have the 'Atomics' property? I think since $262.agent only gets called by Atomics tests, which all get skipped if 'Atomics' isn't available.

Comment 6

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6efb6e639c02

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1346068-test262-agent-v2.patch

Updated patch, carrying r+.

This now properly guards against the absence of Atomics and SharedArrayBuffer (test with --shared-memory=off to exercise this).  However it seems a number of test cases do not properly guard against that possibility at this stage.

Comment 8

[Lars T Hansen [:lth]]

An additional problem is that test cases that require concurrent agents will fail if the shell is run with --no-threads, see the try run:

test262/shell.js:340:21 Error: Can't create threads with --no-threads

We can maybe use helperThreadCount() > 0 to guard against that, though not with --enable-more-deterministic the way it is now - in that case helperThreadCount returns 0 always, not an optimal situation.  We might not care about --enable-more-deterministic though.

I propose that I implement $262.agent.available() to allow test cases to guard against this easily, presumably this API should be upstreamed?

Comment 9

[Lars T Hansen [:lth]]

I suppose jstests.list and the infrastructure around that could be brought to bear on this problem, but only if it passes arguments like --no-threads and --shared-memory=off on to the shell that it's using for feature testing.

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1346068-test262-agent-v3.patch

Also return the dummy object if helperThreadCount() == 0; improve error messages thrown by the dummy object.

Comment 11

[Lars T Hansen [:lth]]

OK, so:

* --enable-more-deterministic can be handled by jstests.list because in this case helperThreadCount() == 0 always
* --shared-memory=off is not a configuration we're currently testing so we can ignore it
* --no-threads is a real pain

The problem with --no-threads is that it modifies the APIs offered by the shell when run, but the test harness assumes that the value returned by XULInfoTester is valid regardless of shell arguments.  This is clearly wrong but I don't know if that's really what I want to be fixing right now, it looks like the logic might have to change deeply because every filter by the xul_tester would need to take into account the argument combinations.

The simple is to modify the test cases themselves to pass if !$262.agent.available(), which seems somewhat reasonable and portable.  It'll have to be upstreamed, though, along with the definition of available().

Comment 12

[Lars T Hansen [:lth]]

Attachment: bug1346068-test262-agent-v4.patch

Asking for re-review here due to following changes.

As outlined in previous comments, our test runner does not properly filter test cases that are affected by eg --no-threads and it's a lot of work to fix that for this corner case.  It's also not appealing to add hacks to the tests themselves to deal with this case.  So what I've done here is to add a guard to our implementation of $262.agent: if Atomics and SharedArrayBuffer are available, but we can't run agents because helper threads are disabled in the engine, then we silently succeed the test.

This also handles the case where the shell is built with --enable-more-deterministic, in which case the number of helper threads is pinned to zero.  (That case could be handled in jstests.list, but there's no obvious reason to.)

Anyway, with these changes nothing needs to be upstreamed.

Comment 13

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6396a5387c1c

Comment 14

[Lars T Hansen [:lth]]

Comment on attachment 8849502 [details] [diff] [review]
bug1346068-test262-agent-v4.patch

Several rounds on try later I'm in still in the weeds.

These tests were never written to work in a browser, and indeed there are several reasons why they cannot reliably do so.

For one, Firefox requires that the creator of a Worker backs out to the event loop after creating the Worker before the Worker will start.  These tests assume that start() will start the worker and the test can just forge ahead; it just ain't so.  At a minimum, start() must take a continuation to be invoked once the worker is properly up.  (Ben told me at some point he thinks Chrome may have the same restriction.)

For another, sleep() is used on the main thread and this is OK for a shell but probably a problem in the browser: if the main thread is busy-waiting for something to happen on the worker and the worker happens to need the main thread to accomplish something.  And busy-waiting is the only way to implement sleep on the main thread.

So for the time being I will offer up another patch that makes the test cases work in the shell but which makes them silently succeed in the browser, and then I will lift the matter up to the test262 group about making the tests browser-friendly.

Comment 15

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9350a9e89eb115c6c93878f7b4350cb19772ba95

Comment 16

[Shu-yu Guo [:shu]]

(In reply to Lars T Hansen [:lth] from comment #14)
> Comment on attachment 8849502 [details] [diff] [review]
> bug1346068-test262-agent-v4.patch
> 
> Several rounds on try later I'm in still in the weeds.
> 
> These tests were never written to work in a browser, and indeed there are
> several reasons why they cannot reliably do so.
> 
> For one, Firefox requires that the creator of a Worker backs out to the
> event loop after creating the Worker before the Worker will start.  These
> tests assume that start() will start the worker and the test can just forge
> ahead; it just ain't so.  At a minimum, start() must take a continuation to
> be invoked once the worker is properly up.  (Ben told me at some point he
> thinks Chrome may have the same restriction.)
> 
> For another, sleep() is used on the main thread and this is OK for a shell
> but probably a problem in the browser: if the main thread is busy-waiting
> for something to happen on the worker and the worker happens to need the
> main thread to accomplish something.  And busy-waiting is the only way to
> implement sleep on the main thread.
> 
> So for the time being I will offer up another patch that makes the test
> cases work in the shell but which makes them silently succeed in the
> browser, and then I will lift the matter up to the test262 group about
> making the tests browser-friendly.

Oof that sucks. Should be worthwhile to land something to enable coverage in the shell, I think.

Comment 17

[Lars T Hansen [:lth]]

Attachment: bug1346068-test262-agent-v5.patch

Finally green, and actually not all that messy.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e8bbbe07d126c8f3110cce23a81119c553beac2

Two key points here: jstests.list disable the tests in non-shell runs, and $262.agent itself quietly passes a test if helper threads are disabled by a command line switch in the shell.

Comment 18

[Shu-yu Guo [:shu]]

Comment on attachment 8850414 [details] [diff] [review]
bug1346068-test262-agent-v5.patch

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

Great, thanks. r=me with nitty renames.

::: js/src/tests/jstests.list
@@ +839,5 @@
> +skip-if(!this.setSharedArrayBuffer) script test262/built-ins/Atomics/wake/wake-nan.js
> +skip-if(!this.setSharedArrayBuffer) script test262/built-ins/Atomics/wake/wake-two.js
> +skip-if(!this.setSharedArrayBuffer) script test262/built-ins/Atomics/wake/wake-in-order.js
> +skip-if(!this.setSharedArrayBuffer) script test262/built-ins/Atomics/wake/wake-one.js
> +skip-if(!this.setSharedArrayBuffer) script test262/built-ins/Atomics/wake/wake-all-on-loc.js

It's more obvious to do |skip-if(!xulRuntime.shell)|.

::: js/src/tests/test262-host.js
@@ +20,5 @@
> +    // The $262.agent framework is not appropriate for browsers yet, and some
> +    // test cases can't work in browsers (they block the main thread).
> +
> +    var shellCode = hasMailbox && hasEvalInWorker;
> +    var available = Atomics && SharedArrayBuffer && hasThreads && shellCode;

These two variables are agent-specific. Rename something like sabTestable?

@@ +51,5 @@
> +                    _notAvailable() {
> +                        // See comment above.
> +                        if (!hasThreads && shellCode) {
> +                            global.reportCompare(0,0);
> +                            global.quit(0);

Heh, clever.

Comment 19

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83dc915e9ffb44049fb7c578e644ef4a69638a0a

Comment 20

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e32c78f542c68e575b31e242cc0530b76e6e9573
Bug 1346068 - Implement $262.agent.  r=shu

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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