Merge PerThreadDataFriendFields and ContextFriendFields

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Created attachment 8780055 [details] [diff] [review]
Patch

A lot of goodness comes from this:

* Moving the root lists and stack limits to ContextFriendFields makes accessing them more efficient (it eliminates the cx->runtime_ dereference).

* It simplifies the rooting APIs: it's now only possible to construct Rooted<> and PersistentRooted<> from the {Rooting,Exclusive,JS}Context classes. It's no longer possible to use PerThreadData or JSRuntime for this.

* It lets us remove PerThreadDataFriendFields, the js::GetRuntime friend API, etc.

* 217 insertions(+), 254 deletions(-)
Attachment #8780055 - Flags: review?(terrence)
Attachment #8780055 - Flags: review?(bzbarsky)
(Assignee)

Comment 1

a year ago
bz, the changes to the dom/ code are just to get rid of the js::GetRuntime call hidden there. I replaced the JSRuntime* with a JS::RootingContext* (and a debug-only mCalledInit bit).
Comment on attachment 8780055 [details] [diff] [review]
Patch

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

This is great!

::: js/public/RootingAPI.h
@@ +666,5 @@
> +    inline js::RootedListHeads& rootLists(js::ContextFriendFields* cx) {
> +        if (JS::Zone* zone = cx->zone_)
> +            return JS::shadow::Zone::asShadowZone(zone)->stackRoots_;
> +        MOZ_ASSERT(cx->isJSContext);
> +        return cx->roots.stackRoots_;

Yup! It should be only globals (and maybe some atoms) that get touched when we are outside JSAutoCompartment and these all very much happen on the main thread.

@@ +994,4 @@
>      }
>  
> +    // Disallow ExclusiveContext*.
> +    js::RootLists& rootLists(js::ContextFriendFields* cx) = delete;

\o/

::: js/src/gc/Zone.cpp
@@ +299,5 @@
>  Zone::notifyObservingDebuggers()
>  {
>      for (CompartmentsInZoneIter comps(this); !comps.done(); comps.next()) {
>          JSRuntime* rt = runtimeFromAnyThread();
> +        RootedGlobalObject global(rt->contextFromMainThread(), comps->unsafeUnbarrieredMaybeGlobal());

It worries me that we used runtimeFromAnyThread in the line above this rather than runtimeFromMainThread.

::: js/src/jspubtd.h
@@ -449,5 @@
> -            reinterpret_cast<const char*>(rt) + RuntimeMainThreadOffset);
> -    }
> -
> -    template <typename T> friend class JS::Rooted;
> -};

\o/ \o/ \o/

All this complexity when we could have just passed the stack limit around!
Attachment #8780055 - Flags: review?(terrence) → review+
Comment on attachment 8780055 [details] [diff] [review]
Patch

r=me on the DOM bits.
Attachment #8780055 - Flags: review?(bzbarsky) → review+

Comment 4

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e0d9eb7eb05
Merge PerThreadDataFriendFields and ContextFriendFields, clean up APIs. r=terrence,bz

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e0d9eb7eb05
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.