Closed
Bug 1281529
Opened 8 years ago
Closed 8 years ago
Make JSContext inherit from JSRuntime
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(9 files, 1 obsolete file)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | 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 1•8 years ago
|
||
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+
Assignee | ||
Comment 2•8 years ago
|
||
(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
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8764918 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8764919 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8764942 -
Attachment is obsolete: true
Attachment #8764942 -
Flags: review?(luke)
Attachment #8764943 -
Flags: review?(luke)
Comment 11•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8764968 -
Flags: review?(terrence) → review+
Comment 14•8 years ago
|
||
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/498dfbe07a6c
part 1 - Make JSContext inherit from JSRuntime. r=luke
Comment 15•8 years ago
|
||
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
Comment 16•8 years ago
|
||
bugherder |
Updated•8 years ago
|
Attachment #8764932 -
Flags: review?(hv1989) → review+
Comment 17•8 years ago
|
||
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d28e336d47
part 2 - Make cx->runtime() return |this|. r=jorendorff
Assignee | ||
Comment 18•8 years ago
|
||
(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•8 years ago
|
||
bugherder |
Comment 20•8 years ago
|
||
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+
Comment 21•8 years ago
|
||
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
Comment 22•8 years ago
|
||
bugherder |
Updated•8 years ago
|
Attachment #8764918 -
Flags: review?(jwalden+bmo) → review+
Comment 23•8 years ago
|
||
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 24•8 years ago
|
||
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+
Comment 25•8 years ago
|
||
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
Comment 26•8 years ago
|
||
bugherder |
Comment 27•8 years ago
|
||
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
Comment 28•8 years ago
|
||
bugherder |
Comment 29•8 years ago
|
||
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
Comment 30•8 years ago
|
||
bugherder |
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
No longer depends on: 1320159
Assignee | ||
Updated•1 year ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•