Closed
Bug 1464134
Opened 6 years ago
Closed 6 years ago
Use Realm more instead of JSCompartment
Categories
(Core :: JavaScript Engine, enhancement, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(8 files)
48.41 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
10.46 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
2.33 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
45.37 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
6.86 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
4.41 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
3.02 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
19.59 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
Bug 1461938 moved most of the fields to JS::Realm, but we still have a bunch of places where we do unnecessary conversions or assume a compartment has a single realm. Many of these are easy to fix up so let's do that now.
Assignee | ||
Comment 1•6 years ago
|
||
Most of these changes are pretty simple. NewCompartment => NewRealm, MergeCompartments => MergeRealms, we now have FrameIter::realm(), etc. This gets rid of a number of GetCompartmentForRealm and GetRealmForCompartment calls and things are starting to look more natural :)
Attachment #8980368 -
Flags: review?(luke)
Updated•6 years ago
|
Attachment #8980368 -
Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf36035bed13 part 1 - Fix various places to use Realm instead of JSCompartment. r=luke
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 3•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf36035bed13
Assignee | ||
Comment 4•6 years ago
|
||
This adds a Vector of Realms to JSCompartment and a RealmsInCompartmentIter iterator class that's very similar to CompartmentsInZoneIter (I considered having them share code but it's probably not worth it).
Attachment #8981119 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 5•6 years ago
|
||
oldCompartment is never set so the LeaveRealm call in JSAPITest::uninit is unreachable.
Attachment #8981120 -
Flags: review?(andrebargull)
Comment 6•6 years ago
|
||
Comment on attachment 8981120 [details] [diff] [review] Part 3 - Remove GetRealmForCompartment calls from jsapi-tests Review of attachment 8981120 [details] [diff] [review]: ----------------------------------------------------------------- Easy!
Attachment #8981120 -
Flags: review?(andrebargull) → review+
Assignee | ||
Comment 7•6 years ago
|
||
This one is kind of nice because it removes 6 calls to GetRealmForCompartment, and there aren't *that* many left at this point.
Attachment #8981124 -
Flags: review?(evilpies)
Assignee | ||
Comment 8•6 years ago
|
||
RealmsIterT still uses GetRealmForCompartment. To fix this, the patch removes RealmsIterT and turns CompartmentsIterT into CompartmentsOrRealmsIterT with an InnerIterT template parameter that's either CompartmentsInZoneIter or RealmsInZoneIter.
Attachment #8981144 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 9•6 years ago
|
||
Also an easy one. This has a drive-by fix for SavedStacks::insertFrames to use iter.realm() instead of iter.compartment().
Attachment #8981148 -
Flags: review?(andrebargull)
Updated•6 years ago
|
Attachment #8981148 -
Flags: review?(andrebargull) → review+
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8981153 -
Flags: review?(luke)
Updated•6 years ago
|
Attachment #8981153 -
Flags: review?(luke) → review+
Assignee | ||
Comment 11•6 years ago
|
||
For the most part pretty straight-forward. One issue is that some of the Realm APIs we want to call take a Handle<Realm*> so I changed IterateRealmCallback to take a Handle<Realm*> instead of a plain Realm*.
Attachment #8981187 -
Flags: review?(jcoppeard)
Comment 12•6 years ago
|
||
Comment on attachment 8981124 [details] [diff] [review] Part 4 - Rename CompileCompartment to CompileRealm Review of attachment 8981124 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CompileWrappers.cpp @@ +235,2 @@ > { > + return reinterpret_cast<JS::Realm*>(this); Is this even safe C++?
Attachment #8981124 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #12) > > + return reinterpret_cast<JS::Realm*>(this); > > Is this even safe C++? I was wondering about that too... It would be nicer to use private inheritance with "using Base::bar;" to expose the white-listed methods, but that's a pre-existing issue for another day :)
Comment 14•6 years ago
|
||
Comment on attachment 8981119 [details] [diff] [review] Part 2 - Add a Vector of Realms to JSCompartment and add RealmsInCompartmentIter Review of attachment 8981119 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/GC.cpp @@ +7966,5 @@ > + JSCompartment* comp = realm->compartment(); > + if (!comp->realms().append(realm)) { > + ReportOutOfMemory(cx); > + return nullptr; > + } This confused me until I realised that realms are still 1:1 with compartments at this point.
Attachment #8981119 -
Flags: review?(jcoppeard) → review+
Comment 15•6 years ago
|
||
Comment on attachment 8981144 [details] [diff] [review] Part 5 - Combine CompartmentsIterT and RealmsIterT in a single template class Review of attachment 8981144 [details] [diff] [review]: ----------------------------------------------------------------- Nice. I sometimes wonder if we should have a library for combining iterators or whether that would be overkill.
Attachment #8981144 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8981187 -
Flags: review?(jcoppeard) → review+
Comment 16•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8de4be88b8ca part 2 - Add a Vector of Realms to JSCompartment and add RealmsInCompartmentIter. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/bdadfc22d3fe part 3 - Remove GetRealmForCompartment calls from jsapi-tests. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/8af7dd4fb5e2 part 4 - Rename CompileCompartment to CompileRealm. r=evilpie
Updated•6 years ago
|
status-firefox62:
--- → fix-optional
Priority: -- → P2
Comment 17•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/51748a8a2967 part 5 - Combine CompartmentsIterT and RealmsIterT in a single template class. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/e73059705526 part 6 - Replace AbstractFramePtr::compartment with AbstractFramePtr::realm. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/24a0788bae5f part 7 - Replace GetAnyCompartmentInZone with GetAnyRealmInZone. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/16106d1e0abd part 8 - Make IterateHeapUnbarriered and related code use realms instead of compartments. r=jonco
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8de4be88b8ca https://hg.mozilla.org/mozilla-central/rev/bdadfc22d3fe https://hg.mozilla.org/mozilla-central/rev/8af7dd4fb5e2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 19•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a42a47e58247 part 1 - Fix various places to use Realm instead of JSCompartment. r=luke
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Pulsebot from comment #19) > Pushed by jandemooij@gmail.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/a42a47e58247 > part 1 - Fix various places to use Realm instead of JSCompartment. r=luke Oops, this belonged to bug 1464374 :/
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/51748a8a2967 https://hg.mozilla.org/mozilla-central/rev/e73059705526 https://hg.mozilla.org/mozilla-central/rev/24a0788bae5f https://hg.mozilla.org/mozilla-central/rev/16106d1e0abd
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a42a47e58247
You need to log in
before you can comment on or make changes to this bug.
Description
•