773049 - Abstract away value forwarding in value wrappers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

We have two classes right now --- Handle<Value> and EncapsulatedValue --- that basically copy-paste the innards of Value in order to forward .whatever() methods to a Value which they wrap.  We also need the same thing for Rooted<Value> and MutableHandle<Value>.  The duplicated code is nasty and makes changes to Value and the other classes error prone, infastructurey and not-likely-to-change-much as they are.  It would be nice if there was a common mechanism that could be shared by all these instances, whether through templates, subclassing or (groan) macros.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch (also introduces a MutableHandle<Value> specialization)

I need MutableHandle<Value> for other reasons and was already working on this sort of thing, so here's a patch.  The reason Handle<Value> is moved is because I want MutableHandle<Value> eventually for CallArgs, so the code needs to move around to not provoke any used-but-not-defined warnings.

I tested the effect of this on compilation time.  Without the patch, a -j1 build takes 7:35 on my system; with the patch it takes 7:51.  Without the patch, a -j8 build takes 2:32, and with it takes 2:31.  So it seems the effect on compilation time of this is happily in the noise.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 641254 [details] [diff] [review]
Patch (also introduces a MutableHandle<Value> specialization)

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

::: js/src/gc/Barrier.h
@@ +303,4 @@
>  template <class T>
>  struct DefaultHasher< HeapPtr<T> > : HeapPtrHasher<T> { };
>  
> +class EncapsulatedValue : public ValueOperations<EncapsulatedValue>

Aren't there still a bunch of Value methods in this class that haven't been removed?

::: js/src/jsapi.h
@@ +876,5 @@
> +};
> +
> +/* This is a specialization of the general Handle template in gc/Root.h. */
> +template <>
> +class Handle<Value> : public ValueOperations<Handle<Value> >

I was hoping to avoid duplication of the guts of Handle and MutableHandle as well.  I think you could fix this here by avoiding template specialization and just having a class HandleValue which inherits from Handle<Value> and ValueOperations<...>.  That means that explicit uses of Handle<Value> won't get the .whatever() methods and HandleValue needs to be used, but oh well.

@@ +958,5 @@
> +    void setBoolean(bool b) { ptr->setBoolean(b); }
> +    void setMagic(JSWhyMagic why) { ptr->setMagic(why); }
> +    bool setNumber(uint32_t ui) { return ptr->setNumber(ui); }
> +    bool setNumber(double d) { return ptr->setNumber(d); }
> +    void setObjectOrNull(JSObject *arg) { ptr->setObjectOrNull(arg); }

Can you factor these into a MutableValueOperations?  These will also be needed for RootedValue.  (Can you add that class too?)

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch, v2

Hmm, yes, MutableValueOperations is a good idea.

Thinking about specializing all this stuff, it occurs to me the previous patch was perhaps misled by how Handle<Value> had previously been specialized.  Rather than specializing Handle, and having to match up the signature of the default Handle template (and its constructors, and copying its internals), we should put the specialization into a *base class* of Handle.  Then the signature and implementation stay the same with no effort at all, and by construction the changes can only be additions.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 641509 [details] [diff] [review]
Patch, v2

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

Thanks!

Comment 5

[Jeff Walden [:Waldo]]

After a try-push to verify it compiled everywhere, I landed this in m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ceaeaa0638f

Comment 6

[Jeff Walden [:Waldo]]

And a typo fix:

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

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6ceaeaa0638f
https://hg.mozilla.org/mozilla-central/rev/a8e4b5602618

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f3534c54fa3