From 4c9a0dab80520f51a09a318f1adaa7ea8c065e17 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Mon, 4 May 2020 16:53:56 +0000 Subject: [PATCH] Bug 1634459 - Simplify the wrapper used for rooting non-GC thing types r=jandem Previously this stored a function pointer to do the tracing along with the rooted thing. The patch changes this to use a virtual method for the tracing. Calling the method on the base class means we don't need to do address arithmetic to find wher the trace function is stored and we don't need the alignment restrictions, because the virtual trace method knows the layout of its class. I had to add a traits class, mainly to get the address of the rooted thing inside the wrapper since we need to be able to get the address of the wrapper itself now for tracing. The wrapper class is renamed from DispatchWrapper to RootedTraceable. Differential Revision: https://phabricator.services.mozilla.com/D73421 --- js/public/RootingAPI.h | 109 ++++++++++++++++++++------------------ js/rust/build.rs | 2 +- js/rust/etc/wrapper.hpp | 4 +- js/src/gc/RootMarking.cpp | 69 +++++++++++++----------- 4 files changed, 99 insertions(+), 85 deletions(-) diff --git a/js/public/RootingAPI.h b/js/public/RootingAPI.h index 403f3973c5df..22daaa7a2751 100644 --- a/js/public/RootingAPI.h +++ b/js/public/RootingAPI.h @@ -874,43 +874,43 @@ struct FallibleHashMethods> { namespace js { -// The alignment must be set because the Rooted and PersistentRooted ptr fields -// may be accessed through reinterpret_cast*>, and -// the compiler may choose a different alignment for the ptr field when it -// knows the actual type stored in DispatchWrapper. -// -// It would make more sense to align only those specific fields of type -// DispatchWrapper, rather than DispatchWrapper itself, but that causes MSVC to -// fail when Rooted is used in an IsConvertible test. +struct VirtualTraceable { + virtual ~VirtualTraceable() = default; + virtual void trace(JSTracer* trc, const char* name) = 0; +}; + template -class alignas(8) DispatchWrapper { +struct RootedTraceable final : public VirtualTraceable { static_assert(JS::MapTypeToRootKind::kind == JS::RootKind::Traceable, - "DispatchWrapper is intended only for usage with a Traceable"); + "RootedTraceable is intended only for usage with a Traceable"); - using TraceFn = void (*)(JSTracer*, T*, const char*); - TraceFn tracer; - alignas(gc::CellAlignBytes) T storage; + T ptr; - public: template - MOZ_IMPLICIT DispatchWrapper(U&& initial) - : tracer(&JS::GCPolicy::trace), storage(std::forward(initial)) {} + MOZ_IMPLICIT RootedTraceable(U&& initial) : ptr(std::forward(initial)) {} - // Mimic a pointer type, so that we can drop into Rooted. - T* operator&() { return &storage; } - const T* operator&() const { return &storage; } - operator T&() { return storage; } - operator const T&() const { return storage; } + operator T&() { return ptr; } + operator const T&() const { return ptr; } - // Trace the contained storage (of unknown type) using the trace function - // we set aside when we did know the type. - static void TraceWrapped(JSTracer* trc, T* thingp, const char* name) { - auto wrapper = reinterpret_cast( - uintptr_t(thingp) - offsetof(DispatchWrapper, storage)); - wrapper->tracer(trc, &wrapper->storage, name); + void trace(JSTracer* trc, const char* name) override { + JS::GCPolicy::trace(trc, &ptr, name); } }; +template +struct RootedTraceableTraits { + static T* address(RootedTraceable& self) { return &self.ptr; } + static const T* address(const RootedTraceable& self) { return &self.ptr; } + static void trace(JSTracer* trc, VirtualTraceable* thingp, const char* name); +}; + +template +struct RootedGCThingTraits { + static T* address(T& self) { return &self; } + static const T* address(const T& self) { return &self; } + static void trace(JSTracer* trc, T* thingp, const char* name); +}; + } /* namespace js */ namespace JS { @@ -1038,18 +1038,16 @@ class JS_PUBLIC_API AutoGCRooter { namespace detail { -/* - * For pointer types, the TraceKind for tracing is based on the list it is - * in (selected via MapTypeToRootKind), so no additional storage is - * required here. Non-pointer types, however, share the same list, so the - * function to call for tracing is stored adjacent to the struct. Since C++ - * cannot templatize on storage class, this is implemented via the wrapper - * class DispatchWrapper. - */ template -using MaybeWrapped = +using RootedPtr = std::conditional_t::kind == JS::RootKind::Traceable, - js::DispatchWrapper, T>; + js::RootedTraceable, T>; + +template +using RootedPtrTraits = + std::conditional_t::kind == JS::RootKind::Traceable, + js::RootedTraceableTraits, + js::RootedGCThingTraits>; // Dummy types to make it easier to understand template overload preference // ordering. @@ -1069,6 +1067,9 @@ using OverloadSelector = PreferredOverload; */ template class MOZ_RAII Rooted : public js::RootedBase> { + using Ptr = detail::RootedPtr; + using PtrTraits = detail::RootedPtrTraits; + inline void registerWithRootLists(RootedListHeads& roots) { this->stack = &roots[JS::MapTypeToRootKind::kind]; this->prev = *stack; @@ -1143,8 +1144,14 @@ class MOZ_RAII Rooted : public js::RootedBase> { DECLARE_POINTER_CONSTREF_OPS(T); DECLARE_POINTER_ASSIGN_OPS(Rooted, T); - DECLARE_NONPOINTER_ACCESSOR_METHODS(ptr); - DECLARE_NONPOINTER_MUTABLE_ACCESSOR_METHODS(ptr); + + T& get() { return ptr; } + const T& get() const { return ptr; } + + T* address() { return PtrTraits::address(ptr); } + const T* address() const { return PtrTraits::address(ptr); } + + void trace(JSTracer* trc, const char* name); private: /* @@ -1155,7 +1162,7 @@ class MOZ_RAII Rooted : public js::RootedBase> { Rooted** stack; Rooted* prev; - detail::MaybeWrapped ptr; + Ptr ptr; Rooted(const Rooted&) = delete; } JS_HAZ_ROOTED; @@ -1326,6 +1333,8 @@ class PersistentRooted : public js::RootedBase>, private mozilla::LinkedListElement> { using ListBase = mozilla::LinkedListElement>; + using Ptr = detail::RootedPtr; + using PtrTraits = detail::RootedPtrTraits; friend class mozilla::LinkedList; friend class mozilla::LinkedListElement; @@ -1390,7 +1399,7 @@ class PersistentRooted const_cast(rhs).setNext(this); } - bool initialized() { return ListBase::isInList(); } + bool initialized() const { return ListBase::isInList(); } void init(RootingContext* cx) { init(cx, SafelyInitialized()); } void init(JSContext* cx) { init(RootingContext::get(cx)); } @@ -1415,19 +1424,15 @@ class PersistentRooted DECLARE_POINTER_CONSTREF_OPS(T); DECLARE_POINTER_ASSIGN_OPS(PersistentRooted, T); - DECLARE_NONPOINTER_ACCESSOR_METHODS(ptr); - // These are the same as DECLARE_NONPOINTER_MUTABLE_ACCESSOR_METHODS, except - // they check that |this| is initialized in case the caller later stores - // something in |ptr|. + T& get() { return ptr; } + const T& get() const { return ptr; } + T* address() { MOZ_ASSERT(initialized()); - return &ptr; - } - T& get() { - MOZ_ASSERT(initialized()); - return ptr; + return PtrTraits::address(ptr); } + const T* address() const { return PtrTraits::address(ptr); } template void set(U&& value) { @@ -1435,8 +1440,10 @@ class PersistentRooted ptr = std::forward(value); } + void trace(JSTracer* trc, const char* name); + private: - detail::MaybeWrapped ptr; + Ptr ptr; } JS_HAZ_ROOTED; namespace detail { diff --git a/js/rust/build.rs b/js/rust/build.rs index b185b1e65ce6..bffd4bda9e77 100644 --- a/js/rust/build.rs +++ b/js/rust/build.rs @@ -214,7 +214,7 @@ const WHITELIST_TYPES: &'static [&'static str] = &[ "jsid", "JS::Compartment", "JS::Latin1Char", - "JS::detail::MaybeWrapped", + "JS::detail::RootedPtr", "JS::MutableHandle", "JS::MutableHandleObject", "JS::MutableHandleValue", diff --git a/js/rust/etc/wrapper.hpp b/js/rust/etc/wrapper.hpp index 15a506067b66..da66d6cdb20b 100644 --- a/js/rust/etc/wrapper.hpp +++ b/js/rust/etc/wrapper.hpp @@ -31,9 +31,9 @@ typedef uint32_t HashNumber; // Replacements for types that are too difficult for rust-bindgen. -///
+///
template -using replaces_MaybeWrapped = T; +using replaces_RootedPtr = T; ///
struct MutableHandleIdVector_Simple { diff --git a/js/src/gc/RootMarking.cpp b/js/src/gc/RootMarking.cpp index 5c3d6c9c5251..6a50d7fc78d9 100644 --- a/js/src/gc/RootMarking.cpp +++ b/js/src/gc/RootMarking.cpp @@ -31,47 +31,55 @@ using namespace js; using namespace js::gc; +using mozilla::LinkedList; + using JS::AutoGCRooter; using RootRange = RootedValueMap::Range; using RootEntry = RootedValueMap::Entry; using RootEnum = RootedValueMap::Enum; -template -using TraceFunction = void (*)(JSTracer* trc, T* ref, const char* name); - -// For more detail see JS::Rooted::ptr and js::DispatchWrapper. +// For more detail see JS::Rooted::root and js::RootedTraceable. // -// The JS::RootKind::Traceable list contains a bunch of totally disparate -// types, but the instantiations of DispatchWrapper below need /something/ in -// the type field. We use the following type as a compatible stand-in. No -// actual methods from ConcreteTraceable type are actually used at runtime -- -// the real trace function has been stored inline in the DispatchWrapper. +// The JS::RootKind::Traceable list contains a bunch of totally disparate types, +// but to refer to this list we need /something/ in the type field. We use the +// following type as a compatible stand-in. No actual methods from +// ConcreteTraceable type are actually used at runtime. struct ConcreteTraceable { - ConcreteTraceable() { MOZ_CRASH("instantiation of ConcreteTraceable"); } - void trace(JSTracer*) {} + ConcreteTraceable() = delete; + void trace(JSTracer*) = delete; }; template -static inline void TraceStackOrPersistentRoot(JSTracer* trc, T* thingp, - const char* name) { +inline void RootedGCThingTraits::trace(JSTracer* trc, T* thingp, + const char* name) { TraceNullableRoot(trc, thingp, name); } -template <> -inline void TraceStackOrPersistentRoot(JSTracer* trc, ConcreteTraceable* thingp, - const char* name) { - js::DispatchWrapper::TraceWrapped(trc, thingp, name); +template +inline void RootedTraceableTraits::trace(JSTracer* trc, + VirtualTraceable* thingp, + const char* name) { + thingp->trace(trc, name); +} + +template +inline void JS::Rooted::trace(JSTracer* trc, const char* name) { + PtrTraits::trace(trc, &ptr, name); +} + +template +inline void JS::PersistentRooted::trace(JSTracer* trc, const char* name) { + PtrTraits::trace(trc, &ptr, name); } template static inline void TraceExactStackRootList(JSTracer* trc, - JS::Rooted* rooter, + JS::Rooted* listHead, const char* name) { - while (rooter) { - T* addr = reinterpret_cast*>(rooter)->address(); - TraceStackOrPersistentRoot(trc, addr, name); - rooter = rooter->previous(); + auto typedList = reinterpret_cast*>(listHead); + for (JS::Rooted* root = typedList; root; root = root->previous()) { + root->trace(trc, name); } } @@ -86,7 +94,7 @@ static inline void TraceStackRoots(JSTracer* trc, TraceExactStackRootList(trc, stackRoots[JS::RootKind::Value], "exact-value"); - // ConcreteTraceable calls through a function pointer. + // RootedTraceable uses virtual dispatch. JS::AutoSuppressGCAnalysis nogc; TraceExactStackRootList( @@ -103,11 +111,11 @@ static void TraceExactStackRoots(JSContext* cx, JSTracer* trc) { template static inline void TracePersistentRootedList( - JSTracer* trc, mozilla::LinkedList>& list, + JSTracer* trc, LinkedList>& list, const char* name) { - for (PersistentRooted* r : list) { - TraceStackOrPersistentRoot( - trc, reinterpret_cast*>(r)->address(), name); + auto& typedList = reinterpret_cast>&>(list); + for (PersistentRooted* root : typedList) { + root->trace(trc, name); } } @@ -122,7 +130,7 @@ void JSRuntime::tracePersistentRoots(JSTracer* trc) { TracePersistentRootedList(trc, heapRoots.ref()[JS::RootKind::Value], "persistent-value"); - // ConcreteTraceable calls through a function pointer. + // RootedTraceable uses virtual dispatch. JS::AutoSuppressGCAnalysis nogc; TracePersistentRootedList( @@ -135,9 +143,8 @@ static void TracePersistentRooted(JSRuntime* rt, JSTracer* trc) { template static void FinishPersistentRootedChain( - mozilla::LinkedList>& listArg) { - auto& list = - reinterpret_cast>&>(listArg); + LinkedList>& listArg) { + auto& list = reinterpret_cast>&>(listArg); while (!list.isEmpty()) { list.getFirst()->reset(); }