Last Comment Bug 773049 - Abstract away value forwarding in value wrappers
: Abstract away value forwarding in value wrappers
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-11 15:06 PDT by Brian Hackett (:bhackett)
Modified: 2012-07-14 10:02 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (also introduces a MutableHandle<Value> specialization) (9.62 KB, patch)
2012-07-11 16:36 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Patch, v2 (12.99 KB, patch)
2012-07-12 09:50 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2012-07-11 15:06:10 PDT
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] (remove +bmo to email) 2012-07-11 16:36:40 PDT
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.
Comment 2 Brian Hackett (:bhackett) 2012-07-11 16:50:29 PDT
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] (remove +bmo to email) 2012-07-12 09:50:29 PDT
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.
Comment 4 Brian Hackett (:bhackett) 2012-07-12 11:19:34 PDT
Comment on attachment 641509 [details] [diff] [review]
Patch, v2

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

Thanks!
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-12 16:25:03 PDT
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] (remove +bmo to email) 2012-07-12 17:07:29 PDT
And a typo fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e4b5602618
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-07-14 10:02:06 PDT
https://hg.mozilla.org/mozilla-central/rev/9f3534c54fa3

Note You need to log in before you can comment on or make changes to this bug.