Closed Bug 1283855 Opened 4 years ago Closed 4 years ago

Make most of JSAPI take JSContext* instead of JSRuntime*

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(28 files, 2 obsolete files)

2.10 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
12.14 KB, patch
luke
: review+
Details | Diff | Splinter Review
13.50 KB, patch
efaust
: review+
Details | Diff | Splinter Review
14.95 KB, patch
terrence
: review+
Details | Diff | Splinter Review
14.74 KB, patch
arai
: review+
Details | Diff | Splinter Review
7.73 KB, patch
terrence
: review+
Details | Diff | Splinter Review
10.87 KB, patch
sfink
: review+
Details | Diff | Splinter Review
7.14 KB, patch
nbp
: review+
Details | Diff | Splinter Review
6.78 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
13.84 KB, patch
jonco
: review+
Details | Diff | Splinter Review
6.60 KB, patch
terrence
: review+
mccr8
: review+
Details | Diff | Splinter Review
25.25 KB, patch
sfink
: review+
Details | Diff | Splinter Review
11.61 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
10.67 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.73 KB, patch
mrrrgn
: review+
Details | Diff | Splinter Review
6.15 KB, patch
jimb
: review+
Details | Diff | Splinter Review
7.22 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.28 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
10.73 KB, patch
luke
: review+
Details | Diff | Splinter Review
14.76 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.47 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.28 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.71 KB, patch
sfink
: review+
Details | Diff | Splinter Review
36.58 KB, patch
terrence
: review+
Details | Diff | Splinter Review
22.09 KB, patch
jonco
: review+
Details | Diff | Splinter Review
30.19 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
7.75 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
55.46 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Most APIs can now take JSContext instead of JSRuntime. It's nice because after that embedders only have to care about JSContext and no longer need a JSRuntime.

It's not clear if we can hide JSRuntime completely: some public GC inline functions use JSRuntime and there are some other challenges, we'll have to see how it works out.
Attachment #8767173 - Flags: review?(jorendorff)
Attachment #8767174 - Flags: review?(luke)
Attachment #8767176 - Flags: review?(efaustbmo)
Waldo, if you're not comfortable reviewing the XPConnect bits please add an additional r?.
Attachment #8767177 - Flags: review?(jwalden+bmo)
Attachment #8767178 - Flags: review?(arai.unmht)
Attachment #8767179 - Flags: review?(terrence)
Attachment #8767180 - Flags: review?(sphink)
Attachment #8767181 - Flags: review?(nicolas.b.pierron)
Attachment #8767183 - Flags: review?(bbouvier)
Attachment #8767186 - Flags: review?(jcoppeard)
The JS_GetContext calls are not great, but we can remove these later when we store JSContext instead of JSRuntime.
Attachment #8767188 - Flags: review?(terrence)
Attachment #8767190 - Flags: review?(sphink)
This also removes a dead setNativeStackQuota method from devtools/shared/heapsnapshot/tests/gtest/DevTools.h
Attachment #8767191 - Flags: review?(nfitzgerald)
Attachment #8767192 - Flags: review?(shu)
Comment on attachment 8767183 [details] [diff] [review]
Part 9 - asm.js and build id ops

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

Thanks.
Attachment #8767183 - Flags: review?(bbouvier) → review+
Comment on attachment 8767178 [details] [diff] [review]
Part 5 - Warning reporter

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

:)
Attachment #8767178 - Flags: review?(arai.unmht) → review+
Attachment #8767186 - Flags: review?(jcoppeard) → review+
Not super happy with the GetJitContext call in IonBuilder, but it's not the first one and it's not too bad.
Attachment #8767199 - Flags: review?(hv1989)
Attachment #8767200 - Flags: review?(winter2718)
Attachment #8767204 - Flags: review?(luke)
Attachment #8767207 - Flags: review?(jorendorff)
Attachment #8767209 - Flags: review?(luke)
Attachment #8767210 - Flags: review?(dteller)
Attachment #8767211 - Flags: review?(arai.unmht)
Attachment #8767212 - Flags: review?(evilpies)
Attachment #8767213 - Flags: review?(sphink)
Attached patch Part 25 - JS_GCSplinter Review
Attachment #8767214 - Flags: review?(terrence)
Comment on attachment 8767191 [details] [diff] [review]
Part 13 - JS_SetNativeStackQuota

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

r=me for js/ and devtools/ changes

The dom/ changes look fine as well, but I'm not sure if they need actual DOM peer stamps. Its a pretty straightforward change, so... *shrugs*
Attachment #8767191 - Flags: review?(nfitzgerald) → review+
Attached patch Part 26 - More GC APIs (obsolete) — Splinter Review
Attachment #8767218 - Flags: review?(terrence)
Attachment #8767174 - Flags: review?(luke) → review+
Attachment #8767222 - Flags: review?(jcoppeard)
Attachment #8767204 - Flags: review?(luke) → review+
Attachment #8767209 - Flags: review?(luke) → review+
Attachment #8767211 - Flags: review?(arai.unmht) → review+
Attachment #8767173 - Flags: review?(jorendorff) → review+
Attachment #8767207 - Flags: review?(jorendorff) → review+
Attachment #8767180 - Flags: review?(sphink) → review+
Comment on attachment 8767190 [details] [diff] [review]
Part 12 - More GC APIs

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

As you said, the JS_GetContext in nsJSEnvironment.cpp are kind of unfortunate, but fine for now.
Attachment #8767190 - Flags: review?(sphink) → review+
Attachment #8767213 - Flags: review?(sphink) → review+
Attachment #8767179 - Flags: review?(terrence) → review+
Comment on attachment 8767188 [details] [diff] [review]
Part 11 - More GC callbacks

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

Forwarding review to Andrew so that he'll be aware of these changes too.
Attachment #8767188 - Flags: review?(terrence)
Attachment #8767188 - Flags: review?(continuation)
Attachment #8767188 - Flags: review+
Attachment #8767214 - Flags: review?(terrence) → review+
Comment on attachment 8767218 [details] [diff] [review]
Part 26 - More GC APIs

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

GC changes look fine to me. Forwarding to Andrew again to okay the browser side.
Attachment #8767218 - Flags: review?(terrence)
Attachment #8767218 - Flags: review?(continuation)
Attachment #8767218 - Flags: review+
Comment on attachment 8767188 [details] [diff] [review]
Part 11 - More GC callbacks

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

r=me for the non-js/ changes

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +338,5 @@
>    JSRuntime *jsRuntime = xpc::GetJSRuntime();
>    MOZ_ASSERT(jsRuntime != nullptr);
>  
>    // Register a callback to trace wrapped JSObjects.
> +  JSContext* cx = JS_GetContext(jsRuntime);

I'd just inline the definition of |jsRuntime| into here (and just delete the assertion) here and below, as it is not otherwise used. Really it feels like there should be some kind of public xpc::GetJSContext() but I'm not up on the subtleties there.
Attachment #8767188 - Flags: review?(continuation) → review+
Attachment #8767200 - Flags: review?(winter2718) → review+
Comment on attachment 8767218 [details] [diff] [review]
Part 26 - More GC APIs

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

r- for the nsJSEnvironment changes. I'd like to see the updated patch. The rest of the non-js/ changes look fine to me.

::: dom/base/nsJSEnvironment.cpp
@@ +1201,5 @@
>    if (!nsContentUtils::XPConnect() || !sRuntime) {
>      return;
>    }
>  
> +  JSContext* cx = JS_GetContext(sRuntime);

I think it makes sense to create a new global variable sContext and use that, instead of patching in these JS_GetContext calls in a billion places in this file. Just set it appropriately right after each place that sRuntime is set.
Attachment #8767218 - Flags: review?(continuation) → review-
Fair enough. I was planning to rename sRuntime to sContext soon, but it's reasonable to add sContext now.
Attachment #8767218 - Attachment is obsolete: true
Attachment #8767262 - Flags: review?(continuation)
Comment on attachment 8767262 [details] [diff] [review]
Part 26 - More GC APIs

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

Thanks.
Attachment #8767262 - Flags: review?(continuation) → review+
Attachment #8767192 - Flags: review?(shu) → review+
This is slightly simpler.
Attachment #8767199 - Attachment is obsolete: true
Attachment #8767199 - Flags: review?(hv1989)
Attachment #8767398 - Flags: review?(hv1989)
Attachment #8767181 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8767212 - Flags: review?(evilpies) → review+
Attachment #8767398 - Flags: review?(hv1989) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb219e5a1a8
part 1 - Make JS_GetEmptyString take JSContext instead of JSRuntime. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/007ccecf74c4
part 2 - Make some callback setters take JSContext instead of JSRuntime. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1be02c27400
part 5 - Make warning reporter APIs take JSContext instead of JSRuntime. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2279c28733
part 6 - Make JS_AbortIfWrongThread take JSContext instead of JSRuntime. r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/937ce4620f6b
part 7 - Make JS_SetGCZeal take JSContext instead of JSRuntime. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ca4097f77f
part 8 - Make JIT compiler option APIs take JSContext instead of JSRuntime. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f84c1f58b4
part 9 - Make asm.js/buildId op setters take JSContext instead of JSRuntime. r=bbouvier
Keywords: leave-open
Comment on attachment 8767222 [details] [diff] [review]
Part 27 - More GC APIs

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

::: dom/base/nsJSEnvironment.cpp
@@ +2447,5 @@
>  
>    // Let's make sure that our main thread is the same as the xpcom main thread.
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  sPrevGCSliceCallback = JS::SetGCSliceCallback(JS_GetContext(sRuntime), DOMGCSliceCallback);

Nit: this line is longer than 80 characters.
Attachment #8767222 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d03d3e56b764
part 10 - Make some GC callback APIs take JSContext instead of JSRuntime. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dfcdfcdcfaf
part 11 - Make some GC callback APIs take JSContext instead of JSRuntime. r=terrence,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e0f9ee9ec4
part 12 - Make some GC APIs take JSContext instead of JSRuntime. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ad59028e26
part 13 - Make JS_SetNativeStackQuota take JSContext instead of JSRuntime. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a08957a6aff
part 14 - Make more callback setters take JSContext instead of JSRuntime. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/cede06835eb1
part 15 - Make {Get,Set}DOMCallbacks, SetWindowProxyClass take JSContext instead of JSRuntime. r=h4writer
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e94e4eb877c
part 16 - Make source hook functions take JSContext instead of JSRuntime. r=mrrrgn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae90cded5407
part 18 - Make JS_DropPrincipals take JSContext instead of JSRuntime. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d7ff2d4bd8
part 19 - Make security callbacks take JSContext instead of JSRuntime. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc9d8139086
part 20 - Make more principals code take JSContext instead of JSRuntime. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee13ff3e4d8
part 22 - Make GetErrorTypeName take JSContext instead of JSRuntime. r=arai
Attachment #8767203 - Flags: review?(jimb) → review+
Comment on attachment 8767176 [details] [diff] [review]
Part 3 - More callbacks

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

::: js/src/shell/js.cpp
@@ +709,1 @@
>                                             ForwardingPromiseRejectionTrackerCallback);

This still doesn't fit on one line, huh? Bummer.
Attachment #8767176 - Flags: review?(efaustbmo) → review+
Jan, can you confirm that there is a 1:1 mapping between JSContext and thread-running-JS? If so, you can turn this f+ in a r+.
Flags: needinfo?(jdemooij)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09d648d9e6e
part 3 - Make some callback setters take JSContext instead of JSRuntime. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/dadebac6b94d
part 17 - Make {Get,Set}DebuggerMallocSizeOf take JSContext instead of JSRuntime. r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a07acf6d4de5
part 23 - Make more callback setters take JSContext instead of JSRuntime. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/e67c9acccc93
part 24 - Make js::DumpHeap take JSContext instead of JSRuntime. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb9bb173e50
part 25 - Make JS_GC take JSContext instead of JSRuntime. r=terrence
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #46)
> Jan, can you confirm that there is a 1:1 mapping between JSContext and
> thread-running-JS? If so, you can turn this f+ in a r+.

Yup, that's correct.
Flags: needinfo?(jdemooij)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fdce8a8b920
part 21 - Make performance monitoring APIs take JSContext instead of JSRuntime. r=Yoric
This covers (almost) all the remaining GC APIs. I tried to do this incrementally but patches modified a lot of the same code, so I folded them to make it easier to review.

I think this is the last patch for this bug - there's more to do but I'll do that in separate bugs because it's less trivial.
Attachment #8768433 - Flags: review?(terrence)
Attachment #8768433 - Flags: review?(terrence) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a064d2e7a3c0
part 26 - Make more GC APIs take JSContext instead of JSRuntime. r=terrence,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22e5cad510b
part 27 - Make more GC APIs take JSContext instead of JSRuntime. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/c521197a29b7
part 28 - Make more GC APIs take JSContext instead of JSRuntime. r=terrence
Comment on attachment 8767177 [details] [diff] [review]
Part 4 - Locale APIs

Switching r? to bholley as it's XPConnect-ish and so I can maybe close this bug this week.
Attachment #8767177 - Flags: review?(jwalden+bmo) → review?(bobbyholley)
Comment on attachment 8767177 [details] [diff] [review]
Part 4 - Locale APIs

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

Stealing review. I haven't worked in this specific part of XPConnect before, but it's a simple enough patch that I don't think it matters too much.
Attachment #8767177 - Flags: review?(bobbyholley) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d8c5ffe860
part 4 - Make localization APIs take JSContext instead of JSRuntime. r=terrence
Keywords: leave-open
Summary: Make JSAPI take JSContext* instead of JSRuntime* → Make most of JSAPI take JSContext* instead of JSRuntime*
https://hg.mozilla.org/mozilla-central/rev/e8d8c5ffe860
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d9c8c76813
OOMTest: Guard JS_DEFAULT_ZEAL_FREQ macro with JS_GC_ZEAL. r=jandem
You need to log in before you can comment on or make changes to this bug.