Closed Bug 1337117 Opened 3 years ago Closed 3 years ago

Remove references to main thread in the JS engine

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
After the previous patches in bug 1323066 there is no longer a concept of a main thread in the JS engine, and it would be good to remove all the old references to a 'main' thread in function names and comments, as this patch does.  Most references are just changed to refer to the runtime's current active thread, which is a similar concept.
Attachment #8834111 - Flags: review?(jdemooij)
(In reply to Brian Hackett (:bhackett) from comment #0)
> Most references are just changed to refer to the runtime's current
> active thread, which is a similar concept.

Will this require changes once there will be multiple threads running JS in parallel?

"active thread" seems confusing: when we MOZ_ASSERT(foo->onActiveThread()), it's not immediately clear what it means. Helper threads are also active in a sense. What about "owner thread"?
(In reply to Jan de Mooij [:jandem] from comment #1)
> (In reply to Brian Hackett (:bhackett) from comment #0)
> > Most references are just changed to refer to the runtime's current
> > active thread, which is a similar concept.
> 
> Will this require changes once there will be multiple threads running JS in
> parallel?

Yeah, when we have preemptive multithreading the concept of an active thread will be no more and these places will require changing again.  Many of the APIs can be simplified, since the runtimeFrom{Any,Main/Active}Thread doesn't matter when the method can be called on multiple threads simultaneously.  I thought about making that change now but it's nice to keep the asserts.

> "active thread" seems confusing: when we MOZ_ASSERT(foo->onActiveThread()),
> it's not immediately clear what it means. Helper threads are also active in
> a sense. What about "owner thread"?

Hmm, I'm not wedded to 'active thread' but 'owner thread' seems to imply some permanence like what 'main thread' did.  Would it help to both (a) add a comment to Runtime.h describing the concepts and restrictions here, and (b) rename these methods to 'onActiveCooperatingThread()' and such?
(In reply to Brian Hackett (:bhackett) from comment #2)
> Hmm, I'm not wedded to 'active thread' but 'owner thread' seems to imply
> some permanence like what 'main thread' did.  Would it help to both (a) add
> a comment to Runtime.h describing the concepts and restrictions here, and
> (b) rename these methods to 'onActiveCooperatingThread()' and such?

Hm I was thinking about onScriptThread or onActiveJSThread, but these aren't great for wasm..

onActiveCooperatingThread + comments explaining this SGTM, it's less generic than 'active thread'.
Comment on attachment 8834111 [details] [diff] [review]
patch

OK, I'll prepare a new patch.
Attachment #8834111 - Flags: review?(jdemooij)
Attached patch patchSplinter Review
Attachment #8834111 - Attachment is obsolete: true
Attachment #8835005 - Flags: review?(jdemooij)
Comment on attachment 8835005 [details] [diff] [review]
patch

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

Thanks. I only skimmed this because the patch is so big.

::: js/src/builtin/ModuleObject.cpp
@@ +607,5 @@
>  
>  /* static */ void
>  ModuleObject::finalize(js::FreeOp* fop, JSObject* obj)
>  {
> +    MOZ_ASSERT(fop->maybeOnBackgroundThread());

Nit: maybeOnHelperThread for consistency with other code where we call them helper threads?

::: js/src/jit/mips32/Bailouts-mips32.cpp
@@ +31,5 @@
>          return;
>      }
>  
>      // Compute the snapshot offset from the bailout ID.
> +    JSRuntime* rt = activation->compartment()->runtimeFromActiveThread();

ActiveCooperating

::: js/src/vm/Initialization.cpp
@@ +95,5 @@
>      RETURN_IF_FAIL(js::TlsContext.init());
>  
>  #if defined(DEBUG) || defined(JS_OOM_BREAKPOINT)
>      RETURN_IF_FAIL(js::oom::InitThreadType());
> +    js::oom::SetThreadType(js::oom::THREAD_TYPE_ACTIVE);

ACTIVE_COOPERATING?

::: js/src/vm/Runtime.h
@@ +139,5 @@
> +//
> +// - Background threads do not run JS, and are controlled or triggered by
> +//   activity in the cooperating threads. Background threads may have exclusive
> +//   access to zone groups created for them, for parsing and similar tasks, but
> +//   their activities do not cause observable changes in script behaviors.

Same here, we should call them either background threads or helper threads everywhere.

::: js/src/vm/TraceLogging.h
@@ +304,5 @@
>      bool initialized;
>  #endif
>  
>      bool enabledTextIds[TraceLogger_Last];
> +    bool activeThreadEnabled;

activeCooperatingThreadEnabled or cooperatingThreadEnabled
Attachment #8835005 - Flags: review?(jdemooij) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7002008dc116
Remove references to main thread in the JS engine, r=jandem.
https://hg.mozilla.org/mozilla-central/rev/7002008dc116
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.