1157533 - Share GC pointer dispatch logic for Value and jsid

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 10.0.0_shared_value_and_id_dispatch-v0.diff

We repeat the fiddly |if (val->isFoo()) DoSomething(val->asFoo())| all over the place. It would be better to share them. This seems to work locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44a7bf4d4f54

Comment 1

[Terrence Cole [:terrence]]

Attachment: 10.0.0_shared_value_and_id_dispatch-v1.diff

The second try run from last night turned out quite green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84bfd5cc4a3f

However, I thought of a bunch of improvements this morning. The new patch is quite a bit nicer. New try run up to verify that I didn't break anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41644ff5013c

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8596700 [details] [diff] [review]
10.0.0_shared_value_and_id_dispatch-v1.diff

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

I really like the DefaultAdaptors, that's a neat way of handling it.

::: js/public/Value.h
@@ +1878,5 @@
> +    return F::defaultValue(val);
> +}
> +
> +template <class S> struct VoidDefaultAdaptor { static void defaultValue(S) {} };
> +template <class S> struct CopyDefaultAdaptor { static S defaultValue(const S &v) { return v; } };

You could call this one IdentityDefaultAdapator.

::: js/src/gc/Marking.h
@@ +171,5 @@
> +#define DECLARE_REWRAP(S, T, method, prefix) \
> +    template <> struct RewrapValueOrId<S, T> { \
> +        static S wrap(T thing) { return method ( prefix thing ); } \
> +    }
> +DECLARE_REWRAP(JS::Value, JSObject*, JS::ObjectValue, *);

Do we ever try and re-wrap a null object pointer?  If so you could use ObjectOrNullValue here.

Comment 3

[Pulsebot]

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

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41644ff5013c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e50b26a090

Comment 5

[Carsten Book [:Tomcat]]

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