1337491 - Off thread parsing changes for multithreaded runtimes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Some changes are necessary to how off thread parsing is structured so that it fits in better with the new runtime structure being developed in bug 1323066:

- Separate zone groups should be used for the zones used in off thread parsing, so that both the active thread and the parsing thread have exclusive access to the zone group they are operating on.

- JSContexts for the parse should not be created on the active thread before running on the helper thread; the helper thread's context can be used instead.  This allows us to ensure that each thread has at most one JSContext at a time.

The attached patch makes these changes.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: nursery changes

Use a separate zone group with a disabled nursery for off thread parses.  Since cx->nursery() is disabled on the parse thread, this patch is also able to remove some crud in the nursery code for avoiding use by parse threads.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: zone changes

This patch moves the flag for whether something is owned by a parse thread from Zone to ZoneGroup, which better reflects the new state of things.  It also renames related methods and fields and makes a few minor fixes.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Parse task changes

This patch avoids creating separate JSContexts for use by parse tasks.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8834573 [details] [diff] [review]
nursery changes

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

Looks good.  Thanks for the nursery cleanups.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8834576 [details] [diff] [review]
zone changes

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

::: js/src/gc/ZoneGroup.h
@@ +44,5 @@
>      // The context with exclusive access to this zone group.
>      UnprotectedData<CooperatingContext> ownerContext_;
>  
>      // The number of times the context has entered this zone group.
> +    UnprotectedData<size_t> enterCount;

Why can't this be protected any more?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Jon Coppeard (:jonco) from comment #5)
> Comment on attachment 8834576 [details] [diff] [review]
> zone changes
> 
> Review of attachment 8834576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/ZoneGroup.h
> @@ +44,5 @@
> >      // The context with exclusive access to this zone group.
> >      UnprotectedData<CooperatingContext> ownerContext_;
> >  
> >      // The number of times the context has entered this zone group.
> > +    UnprotectedData<size_t> enterCount;
> 
> Why can't this be protected any more?

As for |ownerContext_| itself, |enterCount| is accessed when a thread tries to enter a zone group, before it actually has ownership of it.

Right now, entering a zone group is not threadsafe: if two threads try to simultaneously enter a zone group, we might end up with a messed up state.  In a cooperatively multithreaded runtime this can't happen; only one cooperative thread will be operating at a time, plus helper threads for parse tasks that only enter a particular zone group for their task.

Even so, once the functional patches for a cooperatively multithreaded runtime are done (getting close now!) I'll be working on replacing these UnprotectedData uses with more stringent protection checks.  In this case, it would probably be best to go ahead and make zone group transitions threadsafe, to reduce the work required later for having a preemptively multithreaded runtime.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8834577 [details] [diff] [review]
Parse task changes

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

Nice cleanup.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: followup

This bug adds uses of AutoCompartment on helper threads for parsing, which used to work but now doesn't after bug 1337502.  This patch relaxes the gray thing assertions so that AutoCompartment can be used by helper threads again.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ded6149dc4a
Off thread parsing changes for multithreaded runtimes, r=jandem,jonco.

Comment 10

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

Backed out for failing xpcshell tests test_utils_pbkdf2.js and test_ext_runtime_onInstalled_and_onStartup.js on Android 4.3 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e719b89ea8f22b00fb6a52fb962090bbe5c7ac6a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4ded6149dc4a3bbaeb93bd644f927bac37db1cb2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log test_utils_pbkdf2.js: https://treeherder.mozilla.org/logviewer.html#?job_id=77261680&repo=mozilla-inbound
[task 2017-02-14T17:25:24.832350Z] 17:25:24     INFO -  TEST-START | services/crypto/tests/unit/test_utils_pbkdf2.js
[task 2017-02-14T17:30:24.832792Z] 17:30:24  WARNING -  TEST-UNEXPECTED-TIMEOUT | services/crypto/tests/unit/test_utils_pbkdf2.js | Test timed out
[task 2017-02-14T17:30:24.834462Z] 17:30:24     INFO -  TEST-INFO took 300001ms
[task 2017-02-14T17:30:25.152602Z] 17:30:25     INFO -  xpcshell return code: -1
[task 2017-02-14T17:30:34.859570Z] 17:30:34     INFO -  mozdevice Timeout exceeded for shell call 'adb shell /data/local/xpcb/xpcw /storage/sdcard/tests/xpc/services/crypto/tests/unit -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e 'const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js";' -e 'const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json";' -e 'const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m";' -f /storage/sdcard/tests/xpc/head.js -e 'const _SERVER_ADDR = "localhost"' -e 'const _HEAD_FILES = ["/storage/sdcard/tests/xpc/services/crypto/tests/unit/head_helpers.js", "/storage/sdcard/tests/xpc/services/crypto/tests/unit/../../../common/tests/unit/head_helpers.js"];' -e 'const _JSDEBUGGER_PORT = 0;' -e 'const _TEST_FILE = ["test_utils_pbkdf2.js"];' -e 'const _TEST_NAME = "services/crypto/tests/unit/test_utils_pbkdf2.js"' -e '_execute_test(); quit(0);'; echo $?'
[task 2017-02-14T17:30:35.467273Z] 17:30:35     INFO -  services/crypto/tests/unit/test_utils_pbkdf2.js | Process still running after test!
[task 2017-02-14T17:30:35.467477Z] 17:30:35     INFO -  >>>>>>>
[task 2017-02-14T17:30:35.467841Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | xpcw: cd /storage/sdcard/tests/xpc/services/crypto/tests/unit
[task 2017-02-14T17:30:35.468712Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/services/crypto/tests/unit/head_helpers.js", "/storage/sdcard/tests/xpc/services/crypto/tests/unit/../../../common/tests/unit/head_helpers.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_utils_pbkdf2.js"]; -e const _TEST_NAME = "services/crypto/tests/unit/test_utils_pbkdf2.js" -e _execute_test(); quit(0);
[task 2017-02-14T17:30:35.469114Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | [1883] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 172
[task 2017-02-14T17:30:35.469189Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | Warning: MOZILLA_FIVE_HOME not set.
[task 2017-02-14T17:30:35.469386Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | [1883] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /home/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2879
[task 2017-02-14T17:30:35.471889Z] 17:30:35     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-02-14T17:30:35.471960Z] 17:30:35     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2017-02-14T17:30:35.472014Z] 17:30:35     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-02-14T17:30:35.472060Z] 17:30:35     INFO -  running event loop
[task 2017-02-14T17:30:35.472123Z] 17:30:35     INFO -  services/crypto/tests/unit/test_utils_pbkdf2.js | Starting test_pbkdf2
[task 2017-02-14T17:30:35.472174Z] 17:30:35     INFO -  (xpcshell/head.js) | test test_pbkdf2 pending (2)
[task 2017-02-14T17:30:35.472288Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | [1883] WARNING: NSS will be initialized without a profile directory. Some things may not work as expected.: file /home/worker/workspace/build/src/security/manager/ssl/nsNSSComponent.cpp, line 1671
[task 2017-02-14T17:30:35.472392Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | [1883] WARNING: site security information will not be persisted: file /home/worker/workspace/build/src/security/manager/ssl/nsSiteSecurityService.cpp, line 370
[task 2017-02-14T17:30:35.472462Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2 - [test_pbkdf2 : 17] 16 == 16
[task 2017-02-14T17:30:35.472543Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2 - [test_pbkdf2 : 18] "d2zG0d2cBfXnRwMUGyMwyg==" == "d2zG0d2cBfXnRwMUGyMwyg=="
[task 2017-02-14T17:30:35.472629Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2 - [test_pbkdf2 : 19] "O5WMNUO5TQC7LZ2HAMKBWIZQZI======" == "O5WMNUO5TQC7LZ2HAMKBWIZQZI======"
[task 2017-02-14T17:30:35.472697Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2 - [test_pbkdf2 : 21] 32 == 32
[task 2017-02-14T17:30:35.472903Z] 17:30:35     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2017-02-14T17:30:35.473240Z] 17:30:35     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2017-02-14T17:30:35.473580Z] 17:30:35     INFO -  (xpcshell/head.js) | test test_pbkdf2 finished (2)
[task 2017-02-14T17:30:35.473939Z] 17:30:35     INFO -  services/crypto/tests/unit/test_utils_pbkdf2.js | Starting test_pbkdf2_hmac_sha1
[task 2017-02-14T17:30:35.474278Z] 17:30:35     INFO -  (xpcshell/head.js) | test test_pbkdf2_hmac_sha1 pending (2)
[task 2017-02-14T17:30:35.474812Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2_hmac_sha1 - [test_pbkdf2_hmac_sha1 : 92] "0c60c80f961f0e71f3a9b524af6012062fe037a6" == "0c60c80f961f0e71f3a9b524af6012062fe037a6"
[task 2017-02-14T17:30:35.475181Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2_hmac_sha1 - [test_pbkdf2_hmac_sha1 : 92] "ea6c014dc72d6f8ccd1ed92ace1d41f0d8de8957" == "ea6c014dc72d6f8ccd1ed92ace1d41f0d8de8957"
[task 2017-02-14T17:30:35.475422Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2_hmac_sha1 - [test_pbkdf2_hmac_sha1 : 92] "4b007901b765489abead49d926f721d065a429c1" == "4b007901b765489abead49d926f721d065a429c1"
[task 2017-02-14T17:30:35.475876Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2_hmac_sha1 - [test_pbkdf2_hmac_sha1 : 92] "3d2eec4fe41c849b80c8d83662c0e44a8b291a964cf2f07038" == "3d2eec4fe41c849b80c8d83662c0e44a8b291a964cf2f07038"
[task 2017-02-14T17:30:35.476297Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2_hmac_sha1 - [test_pbkdf2_hmac_sha1 : 92] "56fa6aa75548099dcc37d7f03425e0c3" == "56fa6aa75548099dcc37d7f03425e0c3"
[task 2017-02-14T17:30:35.476628Z] 17:30:35     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2017-02-14T17:30:35.476946Z] 17:30:35     INFO -  (xpcshell/head.js) | test run_next_test 2 pending (2)
[task 2017-02-14T17:30:35.477269Z] 17:30:35     INFO -  (xpcshell/head.js) | test test_pbkdf2_hmac_sha1 finished (2)
[task 2017-02-14T17:30:35.477603Z] 17:30:35     INFO -  services/crypto/tests/unit/test_utils_pbkdf2.js | Starting test_pbkdf2_hmac_sha256
[task 2017-02-14T17:30:35.477928Z] 17:30:35     INFO -  (xpcshell/head.js) | test test_pbkdf2_hmac_sha256 pending (2)
[task 2017-02-14T17:30:35.478411Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2_hmac_sha256 - [test_pbkdf2_hmac_sha256 : 154] "120fb6cffcf8b32c43e7225256c4f837a86548c92ccc35480805987cb70be17b" == "120fb6cffcf8b32c43e7225256c4f837a86548c92ccc35480805987cb70be17b"
[task 2017-02-14T17:30:35.478900Z] 17:30:35     INFO -  TEST-PASS | services/crypto/tests/unit/test_utils_pbkdf2.js | test_pbkdf2_hmac_sha256 - [test_pbkdf2_hmac_sha256 : 154] "ae4d0c95af6b46d32d0adff928f06dd02a303f8ef3c251dfd6e2d85a95474c43" == "ae4d0c95af6b46d32d0adff928f06dd02a303f8ef3c251dfd6e2d85a95474c43"
[task 2017-02-14T17:30:35.479518Z] 17:30:35     INFO -  PROCESS | services/crypto/tests/unit/test_utils_pbkdf2.js | {"action":"test_status","time":1487093397025,"thread":null,"pid":null,"source":"xpcshell/head.js","test":"services/crypto/tests/unit/test_utils_pbkdf2.js","subtest":"test_pbkdf2_hmac_sha256","status":"PASS","message":"[test_pbkdf2_hmac_sha256 : 154] \\"c5e478d59288c841aa530db6845c4c8d962893a001ce4e11a4963873aa98134a\\" == \\"c5e478d59288c841aa530db6845c4c8d962893a001ce4e11a4963873aa98134a\\""
[task 2017-02-14T17:30:35.479819Z] 17:30:35     INFO -  <<<<<<<

Failure log test_ext_runtime_onInstalled_and_onStartup.js: https://treeherder.mozilla.org/logviewer.html#?job_id=77261702&repo=mozilla-inbound
> TEST-UNEXPECTED-TIMEOUT | toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js | Test timed out

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/795c13350e9a
Off thread parsing changes for multithreaded runtimes, r=jandem,jonco.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/795c13350e9a