Closed Bug 1281529 Opened 8 years ago Closed 8 years ago

Make JSContext inherit from JSRuntime

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(9 files, 1 obsolete file)

Attached patch Part 1 - Inherit from JSRuntime (deleted) — Splinter Review
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: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1320159
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: