Closed Bug 1337491 Opened 3 years ago Closed 3 years ago

Off thread parsing changes for multithreaded runtimes

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(5 files)

Attached patch patchSplinter Review
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.
Attached patch nursery changesSplinter Review
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.
Attachment #8834573 - Flags: review?(jcoppeard)
Attached patch zone changesSplinter Review
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.
Attachment #8834576 - Flags: review?(jcoppeard)
This patch avoids creating separate JSContexts for use by parse tasks.
Attachment #8834577 - Flags: review?(jdemooij)
Comment on attachment 8834573 [details] [diff] [review]
nursery changes

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

Looks good.  Thanks for the nursery cleanups.
Attachment #8834573 - Flags: review?(jcoppeard) → review+
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?
Attachment #8834576 - Flags: review?(jcoppeard) → review+
(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 on attachment 8834577 [details] [diff] [review]
Parse task changes

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

Nice cleanup.
Attachment #8834577 - Flags: review?(jdemooij) → review+
Blocks: 1337968
Attached patch followupSplinter Review
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.
Attachment #8837116 - Flags: review?(jcoppeard)
Attachment #8837116 - Flags: review?(jcoppeard) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ded6149dc4a
Off thread parsing changes for multithreaded runtimes, r=jandem,jonco.
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
Flags: needinfo?(bhackett1024)
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/795c13350e9a
Off thread parsing changes for multithreaded runtimes, r=jandem,jonco.
https://hg.mozilla.org/mozilla-central/rev/795c13350e9a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(bhackett1024)
Depends on: 1341326
You need to log in before you can comment on or make changes to this bug.