Closed Bug 1281529 Opened 3 years ago Closed 3 years ago

Make JSContext inherit from JSRuntime

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(9 files, 1 obsolete file)

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.
Attachment #8764325 - Flags: review?(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)?
Attachment #8764325 - Flags: review?(luke) → review+
(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.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/273b186d22ca
part 1 - Make JSContext inherit from JSRuntime. r=luke
Keywords: leave-open
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.
Attachment #8764917 - Flags: review?(jorendorff)
Attachment #8764918 - Flags: review?(jwalden+bmo)
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.
Attachment #8764919 - Flags: review?(jcoppeard)
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 :)
Attachment #8764923 - Flags: review?(bbouvier)
In cx->currentScript(), we skipped activations from other contexts. With single-JSContext that's unnecessary.

Also, FrameIter no longer has to update data.cx_.
Attachment #8764932 - Flags: review?(hv1989)
Attachment #8764919 - Flags: review?(jcoppeard) → review+
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.
Attachment #8764942 - Flags: review?(luke)
Attachment #8764942 - Attachment is obsolete: true
Attachment #8764942 - Flags: review?(luke)
Attachment #8764943 - Flags: review?(luke)
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.
Attachment #8764917 - Flags: review?(jorendorff) → review+
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.
Attachment #8764968 - Flags: review?(terrence)
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.
Attachment #8765033 - Flags: review?(jorendorff)
Attachment #8764968 - Flags: review?(terrence) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/498dfbe07a6c
part 1 - Make JSContext inherit from JSRuntime. r=luke
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0009a43ad49a
followup - Don't call mallocSizeOf on a base class pointer. r=orange
Attachment #8764932 - Flags: review?(hv1989) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d28e336d47
part 2 - Make cx->runtime() return |this|. r=jorendorff
(In reply to Jason Orendorff [:jorendorff] from comment #11)
> `act->cx() != this` can't be true anymore.

Part 6 gets rid of it :)
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.
Attachment #8765033 - Flags: review?(jorendorff) → review+
Pushed by jandemooij@gmail.com:
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
Attachment #8764918 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8764943 [details] [diff] [review]
Part 7 - Use context instead of runtime in GenerateJitExit

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

Nice!
Attachment #8764943 - Flags: review?(luke) → review+
Comment on attachment 8764923 [details] [diff] [review]
Part 5 - Get rid of rt->jitJSContext

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

Nice simplification.
Attachment #8764923 - Flags: review?(bbouvier) → review+
Pushed by jandemooij@gmail.com:
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
Pushed by jandemooij@gmail.com:
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
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ecb9d04a90
part 9 - Move JSRuntime caches into a new ContextCaches class. r=jorendorff
Depends on: 1283994
No longer depends on: 1283994
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1320159
No longer depends on: 1320159
You need to log in before you can comment on or make changes to this bug.