The default bug view has changed. See this FAQ.

Abstract away value forwarding in value wrappers

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Assigned: Waldo)

Tracking

Other Branch
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments)

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.
Created attachment 641254 [details] [diff] [review]
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #641254 - Flags: review?(bhackett1024)
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?)
Attachment #641254 - Flags: review?(bhackett1024)
Created attachment 641509 [details] [diff] [review]
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.
Attachment #641509 - Flags: review?(bhackett1024)
Comment on attachment 641509 [details] [diff] [review]
Patch, v2

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

Thanks!
Attachment #641509 - Flags: review?(bhackett1024) → review+
Whiteboard: [js:t]
After a try-push to verify it compiled everywhere, I landed this in m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ceaeaa0638f
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla16
And a typo fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e4b5602618
https://hg.mozilla.org/mozilla-central/rev/6ceaeaa0638f
https://hg.mozilla.org/mozilla-central/rev/a8e4b5602618
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/9f3534c54fa3
You need to log in before you can comment on or make changes to this bug.