1169391 - Clear out "reserved" Rooted contents between JSOP_* cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Right now, Interpret() pushes a small pile of Rooted<T> variables onto the runtime lists on entry, then the JSOPs change their values instead of instantiating their own. But they don't clear their values when they're done, which means that you can end up with stale values keeping things alive that do not need to be kept alive.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Use a ReservedRooted class for optimized Rooted use in vm/Interpreter.cpp

Lots of bikeshedding opportunity here. I wasn't very happy to do all the cut & paste implementation of operator blahblah() stuff, and I *really* wasn't happy about defining an operator&. Especially since it's barely used. But anyway, this doesn't seem completely unreasonable.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8612478 [details] [diff] [review]
Use a ReservedRooted class for optimized Rooted use in vm/Interpreter.cpp

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

This is brilliant. I think you may even have saved a few lines, ultimately. At the very least it makes the actual implementation code cleaner.

::: js/public/RootingAPI.h
@@ +75,5 @@
>   *
>   * - MutableHandle<T> is a non-const reference to Rooted<T>. It is used in the
>   *   same way as Handle<T> and includes a |set(const T& v)| method to allow
>   *   updating the value of the referenced Rooted<T>. A MutableHandle<T> can be
> + *   created with an implicit cast from a Rooted<T>*.

\o/

::: js/src/vm/Interpreter.cpp
@@ +1660,5 @@
> + * variables at the beginning, thus inserting them into the Rooted list once
> + * upon entry. ReservedRooted "borrows" a reserved Rooted variable and uses it
> + * within a local scope, resetting the value to nullptr (or the appropriate
> + * equivalent for T) at scope end. This still avoids inserting/removing the
> + * Rooted from the rooter list, but still prevents stale values from being kept

|This still clause, but still clause| reads a bit awkwardly.

@@ +1669,5 @@
> +class ReservedRootedBase {
> +};
> +
> +template<typename T>
> +class ReservedRooted : public ReservedRootedBase<T> {

{ on newline since the body is non-trivial.

@@ +1688,5 @@
> +
> +    T& get() { return savedRoot->get(); }
> +    const T& get() const { return savedRoot->get(); }
> +    T* address() { return savedRoot->address(); }
> +    const T* address() const { return savedRoot->address(); }

Please use the existing shared decls for these where possible. I think something like:

DECLARE_NONPOINTER_ACCESSOR_METHODS(*savedRoot->address());
DECLARE_NONPOINTER_MUTABLE_ACCESSOR_METHODS(*savedRoot->address());

@@ +1691,5 @@
> +    T* address() { return savedRoot->address(); }
> +    const T* address() const { return savedRoot->address(); }
> +
> +    operator const T&() const { return get(); }
> +    operator T&() { return get(); }

And here you can probably get away with:
DECLARE_NONPOINTER_MUTABLE_ACCESSOR_METHODS(T);

@@ +1695,5 @@
> +    operator T&() { return get(); }
> +    const T& operator->() const { return get(); }
> +    operator Handle<T>() { return *savedRoot; }
> +    operator Rooted<T>&() { return *savedRoot; }
> +    ReservedRooted<T>& operator=(const T& p) { *savedRoot = p; return *this; }

DECLARE_NONPOINTER_MUTABLE_ACCESSOR_METHODS(ReservedRooted, T)

You will need to add a set() method for that, though, so it's less win. Not sure how the others fit in. Most of the other wrapper classes needed a few one-offs too, so not a big deal.

@@ +1706,5 @@
> +    friend class ValueOperations<ReservedRooted<Value>>;
> +    const Value* extract() const {
> +        return static_cast<const ReservedRooted<Value>*>(this)->address();
> +    }
> +};

This seems like a ton of boilerplate until you remember how much you get with it automatically. Still might be worth opening a bug to add a MACRO to wrap all of these nearly-identical CRTP definitions.

Comment 3

[Terrence Cole [:terrence]]

Attachment: scope_wrapping_to_get_clang_building-v0.diff

Clang is a bit less flexible with indirect goto -- it needs ReservedRooted to be destructed before we hit any indirect goto (although normal goto is fine). This patch adds the requisite {} to make that happen.

I generated this patch with -w, since that's a ton of spurious indentation change. You can safely assume that I did actually fix up the indentation.

Comment 4

[Steve Fink [:sfink] [:s:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=350c38d51b18

Comment 5

[Steve Fink [:sfink] [:s:]]

Without the unified breakage:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b715231bb68

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c7ec25b894

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9c7ec25b894