1281529 - Make JSContext inherit from JSRuntime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Inherit from JSRuntime

The attached patch makes this change. This is the minimal patch to make it compile - there's a ton of clean up to do after this.

Two annoying things: (1) some names exist in both ExclusiveContext and JSRuntime, so I had to add "using ExclusiveContext::foo" for these, and (2) some functions have overloads for ExclusiveContext* and JSRuntime*, and passing a JSContext* is now ambiguous.

I'll post followup patches to fix some of these issues in a nicer way.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8764325 [details] [diff] [review]
Part 1 - Inherit from JSRuntime

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

(Sorry for the delay, PTO this week.)

::: js/src/jsgc.cpp
@@ +6985,5 @@
>      suppressGC_++;
>  }
>  
> +AutoSuppressGC::AutoSuppressGC(JSContext* cx)
> +  : suppressGC_(cx->runtime()->mainThread.suppressGC)

Why is cx->runtime() necessary (as opposed to cx->mainthread)?

::: js/src/vm/Runtime.cpp
@@ +171,5 @@
>      defaultVersion_(JSVERSION_DEFAULT),
>      ownerThread_(nullptr),
>      ownerThreadNative_(0),
>      tempLifoAlloc(TEMP_LIFO_ALLOC_PRIMARY_CHUNK_SIZE),
> +    context_(cx),

I suppose a later cleanup is to remove the field and use offsetof instead?

::: js/src/vm/Runtime.h
@@ +1492,5 @@
> +    JSRuntime(JSContext* cx, JSRuntime* parentRuntime);
> +
> +    // destroyRuntime is used instead of a destructor, to ensure the downcast
> +    // to JSContext remains valid. The final GC triggered here depends on this.
> +    void destroyRuntime();

Would the need for this be removed once all the relevant state is moved into the JSContext such that ~JSRuntime only operates on things in JSRuntime (and therefore doesn't need the downcast)?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #1)
> Why is cx->runtime() necessary (as opposed to cx->mainthread)?

Good point. cx->runtime() will be killed soon anyway though.

> ::: js/src/vm/Runtime.cpp
> > +    context_(cx),
> 
> I suppose a later cleanup is to remove the field and use offsetof instead?

Yeah I think we can just static_cast from base to derived? Some searching suggests it does the right thing even with multiple inheritance.

> Would the need for this be removed once all the relevant state is moved into
> the JSContext such that ~JSRuntime only operates on things in JSRuntime (and
> therefore doesn't need the downcast)?

Yes I think we can definitely improve on this.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/273b186d22ca
part 1 - Make JSContext inherit from JSRuntime. r=luke

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Make cx->runtime() return |this|

This makes cx->runtime() return |this| instead of cx->runtime_.

At some point we should remove cx->runtime() completely, but until then this gets rid of a dereference and the compiler can emit better code.

I had to fix up some places because of const/non-const differences.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Use static_cast instead of rt->context_

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Remove no longer necessary cx overloads

Some functions have overloads for both JSContext* and JSRuntime*, and the cx one just forwards to the runtime version.

Because JSContext* can now be passed to functions that take JSRuntime*, we can get rid of the JSContext* versions.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Get rid of rt->jitJSContext

This patch:

* Removes rt->jitJSContext.

* Renames GetJSContextFromJitCode to GetJSContextFromMainThread as it no longer has the "called from JIT code" requirement.

* masm.loadJSContext can now bake in the JSContext directly instead of loading it from rt->jitJSContext.

* It removes Activation::prevJitJSContext_, this nicely simplifies wasm::GenerateJitExit.

Pretty happy with this patch because it lets us generate faster JIT code :)

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 6 - rm activation->cx() checks

In cx->currentScript(), we skipped activations from other contexts. With single-JSContext that's unnecessary.

Also, FrameIter no longer has to update data.cx_.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Use context instead of runtime in GenerateJitExit

We can now optimize GenerateJitExit a bit by accessing fields based on the context instead of the runtime. This eliminates a load and we no longer need the AsmJSIonExitRegE3 scratch register.

The "Disable Activation" sequence still uses the runtime (it uses SymbolicAddress::Runtime), we can fix that later when we move the activation stack to the JSContext.

This patch also hides runtime_ from JSContext: it's more efficient to use the base class.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Use context instead of runtime in GenerateJitExit

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8764917 [details] [diff] [review]
Part 2 - Make cx->runtime() return |this|

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

::: js/src/jscntxtinlines.h
@@ +451,1 @@
>      while (act && (act->cx() != this || (act->isJit() && !act->asJit()->isActive())))

`act->cx() != this` can't be true anymore.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Use cx->updateMallocCounter

This replaces some instances of:

  cx->runtime()->updateMallocCounter(cx->zone(), x);

with:

  cx->updateMallocCounter(x);

Same behavior, but it simplifies a later cx->runtime() patch.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Clean up runtime caches

This moves the per-runtime caches (EvalCache, MathCache, etc.) to JSContext.

I added a new ContextCaches class (in vm/Caches.{h,cpp}), so you can do cx->caches.evalCache. I hope we can do similar grouping for most other fields and make JSContext/JSRuntime less monolithic.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/498dfbe07a6c
part 1 - Make JSContext inherit from JSRuntime. r=luke

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0009a43ad49a
followup - Don't call mallocSizeOf on a base class pointer. r=orange

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/498dfbe07a6c
https://hg.mozilla.org/mozilla-central/rev/0009a43ad49a

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d28e336d47
part 2 - Make cx->runtime() return |this|. r=jorendorff

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> `act->cx() != this` can't be true anymore.

Part 6 gets rid of it :)

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/51d28e336d47

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 8765033 [details] [diff] [review]
Part 9 - Clean up runtime caches

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

Nice!

::: js/src/jit/CompileWrappers.cpp
@@ +76,5 @@
>  
>  const void*
>  CompileRuntime::addressOfLastCachedNativeIterator()
>  {
> +    return &static_cast<JSContext*>(runtime())->caches.nativeIterCache.last;

Can this use runtime()->contextFromMainThread() instead?

::: js/src/jit/MCallOptimize.cpp
@@ +430,5 @@
>          return InliningStatus_NotInlined;
>      if (!IsNumberType(callInfo.getArg(0)->type()))
>          return InliningStatus_NotInlined;
>  
> +    const MathCache* cache = GetJSContextFromMainThread()->caches.maybeGetMathCache();

This seems a little funny, but ok.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22dded920775
part 4 - Remove JSContext overloads of some functions that are no longer necessary. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0591f7ad486
part 6 - Remove some now-unnecessary activation->cx() uses. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b7c8956efcb
part 8 - Use cx->updateMallocCounter in a few places. r=terrence

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22dded920775
https://hg.mozilla.org/mozilla-central/rev/f0591f7ad486
https://hg.mozilla.org/mozilla-central/rev/4b7c8956efcb

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 8764943 [details] [diff] [review]
Part 7 - Use context instead of runtime in GenerateJitExit

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

Nice!

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8764923 [details] [diff] [review]
Part 5 - Get rid of rt->jitJSContext

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

Nice simplification.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09ffd37115bb
part 3 - Use static_cast instead of rt->context_. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf462d6d7f9
part 5 - Get rid of rt->jitJSContext. r=bbouvier

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09ffd37115bb
https://hg.mozilla.org/mozilla-central/rev/dcf462d6d7f9

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c507a300667
part 7 - Simplify GenerateJitExit a bit by using the context instead of the runtime. r=luke

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c507a300667

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ecb9d04a90
part 9 - Move JSRuntime caches into a new ContextCaches class. r=jorendorff

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9ecb9d04a90