1643586 - Remove AutoSetHelperThreadContext from GCParallelTask

Status: RESOLVED FIXED

(Core :: JavaScript: GC, task, P3)

Description

[Jan de Mooij [:jandem]]

Longer-term we'd like to get rid of helper thread JSContexts. The two cases where we still rely on them are GCParallelTask and ParseTask. The ParseTask use will be easier to address after Stencil. For GCParallelTask maybe we could add a GCThreadContext TLS class that stores the thread-specific bits we care about and then remove AutoSetHelperThreadContext.

Comment 1

[Jan de Mooij [:jandem]]

I suspect this would be a nice refactoring because it will make it explicit what state is used by off-thread tasks. For example, poking at this a bit, I noticed helper threads currently use AutoDisableProxyCheck but the JSContext flag we set there is only checked on the main thread so this could stay on JSContext/JSRuntime.

Comment 2

[Jon Coppeard (:jonco)]

This sounds good to me.

Comment 3

[Jon Coppeard (:jonco)]

The plan is to move the parts of JSContext that are used by helper threads into JSFreeOp, which already functions as a kind of GC context.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1643586 - Part 1: Remove use of scoped JSFreeOps to poison freed JIT code r?jandem

This replaces such uses with an RAII class where necessary.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1643586 - Part 2: Use one JSFreeOp per thread r?jandem

This uses the context's default free op for background sweeping so there is now
only one free op per thread. This makes all free ops 'default' ones so this
flag is removed. All free ops now have a runtime so maybeOnHelperThread() is
also removed.

Depends on D140188

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1643586 - Part 3: Move JSFreeOp code under js/src/gc r?jandem

JSFreeOp is defined in gc/FreeOp.h so moves its method implementations under gc/ as well.

Depends on D140189

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1643586 - Part 4: Remove flag for disabling compartment checks from JSContext r?jandem

This is only used in one place and it's simpler to check the tracer kind there.

Depends on D140190

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1643586 - Part 5: Replace heap state checks that rely on there being a JSContext for the thread r?jandem

Depends on D140191

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1643586 - Part 6: Stop using JSContext for GC helper thread tasks r?jandem

This patch moves the debug-only GC state from JSContext to JSFreeOp. Support
functions for this move to FreeOp.h and most RAII classes move to GCInternals.h.

The default free op member is removed from JSContext. A free op for the main
thread is added to GCRuntime, and the helper thread system creates a vector of
free ops that can be used by tasks. GCParallelTask takes one of these and sets
it in TLS when running a task.

Use of JSContext by GC helper threads is replaced by JSFreeOp.

Depends on D140192

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a022c7b2bf0c
Part 1: Remove use of scoped JSFreeOps to poison freed JIT code r=jandem
https://hg.mozilla.org/integration/autoland/rev/f40e41f905a4
Part 2: Use one JSFreeOp per thread r=jandem
https://hg.mozilla.org/integration/autoland/rev/619f2aabe4d5
Part 3: Move JSFreeOp code under js/src/gc r=jandem
https://hg.mozilla.org/integration/autoland/rev/b29a261a06ec
Part 4: Remove flag for disabling compartment checks from JSContext r=jandem
https://hg.mozilla.org/integration/autoland/rev/054a08d13999
Part 5: Replace heap state checks that rely on there being a JSContext for the thread r=jandem
https://hg.mozilla.org/integration/autoland/rev/2fa53cd9ac4e
Part 6: Stop using JSContext for GC helper thread tasks r=jandem

Comment 11

[Atila Butkovits]

Backed out for causing SM bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/0350ac4b305a14b0ece1100fd82f7cb97d707d3e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=SsIfsSTyT9G7Sdz2BP7RjA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2fa53cd9ac4e821763dbc4402e58b58184ce230b

Failure log: https://treeherder.mozilla.org/logviewer?job_id=369843618&repo=autoland&lineNumber=14190

https://treeherder.mozilla.org/logviewer?job_id=369846554&repo=autoland&lineNumber=5908
https://treeherder.mozilla.org/logviewer?job_id=369846934&repo=autoland&lineNumber=45318

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c2076b122e0
Part 1: Remove use of scoped JSFreeOps to poison freed JIT code r=jandem
https://hg.mozilla.org/integration/autoland/rev/a6b5cbbdd54b
Part 2: Use one JSFreeOp per thread r=jandem
https://hg.mozilla.org/integration/autoland/rev/8611c3ef5a77
Part 3: Move JSFreeOp code under js/src/gc r=jandem
https://hg.mozilla.org/integration/autoland/rev/f78d139e1d0e
Part 4: Remove flag for disabling compartment checks from JSContext r=jandem
https://hg.mozilla.org/integration/autoland/rev/410d1b57bb09
Part 5: Replace heap state checks that rely on there being a JSContext for the thread r=jandem
https://hg.mozilla.org/integration/autoland/rev/28829816a629
Part 6: Stop using JSContext for GC helper thread tasks r=jandem

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3c2076b122e0
https://hg.mozilla.org/mozilla-central/rev/a6b5cbbdd54b
https://hg.mozilla.org/mozilla-central/rev/8611c3ef5a77
https://hg.mozilla.org/mozilla-central/rev/f78d139e1d0e
https://hg.mozilla.org/mozilla-central/rev/410d1b57bb09
https://hg.mozilla.org/mozilla-central/rev/28829816a629