1337117 - Remove references to main thread in the JS engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Jan de Mooij [:jandem]]

(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"?

Comment 2

[Brian Hackett [Laid off!]]

(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?

Comment 3

[Jan de Mooij [:jandem]]

(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 4

[Brian Hackett [Laid off!]]

Comment on attachment 8834111 [details] [diff] [review]
patch

OK, I'll prepare a new patch.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 6

[Jan de Mooij [:jandem]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7002008dc116
Remove references to main thread in the JS engine, r=jandem.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7002008dc116