Closed Bug 896100 Opened 11 years ago Closed 11 years ago

Implement the C++11 rvalue reference helpers in MFBT

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: ehsan.akhgari, Assigned: jimb)

References

Details

Attachments

(2 files, 7 obsolete files)

Convert all uses of OldMove and MoveRef to true rvalue references and the modern Move and Forward. r=luke
75.36 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Check that AddPtrs are used only with matching Lookup values.
1.35 KB, patch
Details | Diff | Splinter Review 
We cannot use std::move, std::forward and friends because STLport on Android doesn't support them, so we have to implement our own versions.  They are all very easy to implement, so I'm not sure if it makes sense to wrap the STL versions of them where available (they're mostly one-liners.)
Attached patch WIP (obsolete) — — Splinter Review 
This patch tries to port mozilla::Move to move semantics.  I get build errors on HashTableEntry::setLive, but I'm not familiar enough with the code to know how to correctly fix it.

Waldo, do you mind taking a look please?  Thanks!
Attachment #779237 - Flags: feedback?(jwalden+bmo)
Comment on attachment 779237 [details] [diff] [review]
WIP

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

The problem you're hitting with setLive is moderately clear when you consider the error I get with clang:

./dist/include/js/HashTable.h:681:25: error: call to deleted constructor of 'js::HashMapEntry<js::CrossCompartmentKey, js::ReadBarrieredValue>'
        new(mem.addr()) T(u);
                 ^ ~
../jscompartment.h:290:34: note: in instantiation of member function 'js::HashMap<js::CrossCompartmentKey, js::ReadBarrieredValue, js::WrapperHasher, js::SystemAllocPolicy>::remove' requested here
        crossCompartmentWrappers.remove(p);
                                 ^
./dist/include/js/HashTable.h:574:5: note: function has been explicitly marked deleted here
    HashMapEntry(const HashMapEntry &) = delete;
    ^
1 error generated.

in concert with the forwarding problem noted in the rvalue references link I mention a couple times in comments.

Things were fine before with MoveRef, because that worked okay with const U& as a parameter type.  But when there's no MoveRef, only an rvalue reference type, you run into C++11 reference collapsing behavior, and then you get a const HashMapEntry& && reference, which collapses to a const HashMapEntry&, which matches the deleted HashMapEntry(const HashMapEntry&) overload, and doesn't match the HashMapEntry(HashMapEntry&&) overload that you really want it to hit.

The gist of all that is that we need to implement mozilla::Forward and use that here.

::: js/public/HashTable.h
@@ +581,5 @@
> +    HashMapEntry(HashMapEntry&& rhs)
> +      : key(mozilla::Move(rhs.key)), value(mozilla::Move(rhs.value)) { }
> +    HashMapEntry& operator=(HashMapEntry&& rhs)
> +    {
> +      key = mozilla::Move(rhs.key);

Doesn't this need a const_cast<Key&>(key) = ... same as the MapObjects.cpp changes did?

@@ +836,5 @@
>      };
>  
>      // HashTable is movable
> +    HashTable(HashTable&& rhs)
> +      : AllocPolicy(rhs)

This needs a Move().

::: js/src/ion/AsmJS.cpp
@@ +643,5 @@
>        : argTypes_(cx), retType_(retType) {}
> +    Signature(VarTypeVector&& argTypes, RetType retType)
> +      : argTypes_(Move(argTypes)), retType_(retType) {}
> +    Signature(Signature&& rhs)
> +      : argTypes_(Move(rhs.argTypes_)), retType_(rhs.retType_) {}

Putting a Move() around retType_ seems good for consistency.

@@ +657,5 @@
>  
>      bool appendArg(VarType type) { return argTypes_.append(type); }
>      VarType arg(unsigned i) const { return argTypes_[i]; }
>      const VarTypeVector &args() const { return argTypes_; }
> +    VarTypeVector&& extractArgs() { return Move(argTypes_); }

This method is unused, so remove it.  (Also, returning an rvalue reference seems potentially dodgy here.)

@@ +946,5 @@
> +        Func(Func&& rhs)
> +          : name_(rhs.name_),
> +            sig_(Move(rhs.sig_)),
> +            code_(rhs.code_),
> +            compileTime_(rhs.compileTime_)

Put Move() around all of these, and add in an entry for srcOffset_.  Move-constructing is supposed to create a copy, so it seem pretty bad to not copy all fields.

@@ +1038,4 @@
>  
> +        FuncPtrTable(FuncPtrTable&& rhs)
> +          : sig_(Move(rhs.sig_)), mask_(rhs.mask_), globalDataOffset_(rhs.globalDataOffset_),
> +            elems_(Move(rhs.elems_))

Again, I'd Move() all these.

@@ +1061,5 @@
>        public:
> +        ExitDescriptor(PropertyName *name, Signature&& sig)
> +          : name_(name), sig_(Move(sig)) {}
> +        ExitDescriptor(ExitDescriptor&& rhs)
> +          : name_(rhs.name_), sig_(Move(rhs.sig_))

Move()

::: js/src/ion/AsmJSModule.h
@@ +241,5 @@
>  
>        public:
> +        ExportedFunction(ExportedFunction&& rhs)
> +          : name_(rhs.name_),
> +            maybeFieldName_(rhs.maybeFieldName_),

Put mozilla::Move() around rhs.name_ and rhs.maybeFieldName_ so that they get moved rather than copied.  This probably doesn't matter absent GGC, but with GGC, given those fields have interesting destructors, I'd get worried.  Also file a followup to add a move constructor to RelocatablePtr, and CC terrence on that bug.

It's not strictly necessary, but putting mozilla::Move() around returnType_ and hasCodePtr_ and u is probably a good idea for consistency, even if it's harmless without.

::: mfbt/Move.h
@@ +48,5 @@
>   * 2) a way for a type (like Vector) to announce that it can be moved more
>   *    efficiently than it can be copied, and provide an implementation of that
>   *    move operation.
>   *
> + * The Move(T&) function takes a reference to a T, and returns a C++11 rvalue

The Move function takes a value and returns a C++11 rvalue reference...

Technically it takes an rvalue reference and does some reference collapsing that's a bit difficult to explain, so saying it takes T&, or even T&&, is not really correct.  This phrasing, at least, isn't wrong.  :-)

@@ +93,5 @@
>   *
>   *   c2 = Move(c1);
>   *
>   * This destroys c1, moves c1's value to c2, and leaves c1 in an undefined but
>   * destructible state.

Hmm, this should be "This destroys c2, moves c1's...".

@@ +111,3 @@
>   *
>   * especially since GNU C++ fails to notice that this does indeed initialize x
>   * and y, which may matter if they're const.

Let's add this to the end of the comment:

More details about rvalue references and moving than you ever wanted to know are available at <http://thbecker.net/articles/rvalue_references/section_01.html>, if you're curious, or if something breaks and you don't understand why.

::: mfbt/TypeTraits.h
@@ +457,5 @@
>  
> +/**
> + * Converts reference types to the underlying types.
> + *
> + * mozilla::RemoveReference<A>::Type is A;

Add a |struct A {};| above this to be 100% clear that nothing weird like |typedef int& A;| is happening (in which case these asserted results wouldn't be true).

@@ +462,5 @@
> + * mozilla::RemoveReference<A&>::Type is A;
> + * mozilla::RemoveReference<A&&>::Type is A;
> + */
> +
> +template<class A>

Use T, and typename T, instead of A and class A.  (T is a slightly more canonical name for this.  And it's not necessarily going to be a class, and typename doesn't imply that it'll always be a class.  [Sure, class and typename are synonymous here; this is just an extra minor semantic hint.])

::: mfbt/Vector.h
@@ +454,5 @@
>       * Potentially fallible append operations.
>       *
>       * The function templates that take an unspecified type U require a const T&
> +     * or a T&&.  The T&& variants move their operands into the vector, instead
> +     * of copying them.  If they fail, the operand is left unmoved.

Pre-existing, but do a s/T/U/g on this comment if you're changing things.
Attachment #779237 - Flags: feedback?(jwalden+bmo)
Oh, you need to change HashTable's move constructor/assignment to not do rhs-> any more.  (Maybe others?  I only barely noticed that, it's possible I missed another place or so that makes the same mistake.)  I guess nobody's trying to move HashTables around, because that'll self-destruct as soon as it gets instantiated.
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
I'm going to assume that clearing-needinfo-on-CC was unintentional...
Flags: needinfo?(ehsan)
Attached patch Trial fix? (obsolete) — — Splinter Review 
Hmm, not sure if I'm missing something, but here's what I tried, and it doesn't fix the compilation error at all.  I believe there is something wrong with the implementation of setLive itself...
Flags: needinfo?(ehsan)
Flags: needinfo?(jwalden+bmo)
Comment on attachment 784446 [details] [diff] [review]
Trial fix?

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

There are a couple problems with this fix, that I'll spell out in ridiculous detail because this stuff is mind-bending.

Once you fix these issues, tho, you run into a bunch more issues along these lines.  Fundamentally, |U&& u| meaning that |u| is not an rvalue reference affects a *lot* of code that assumed it could use |const U& u| to generically pass movable stuff around.  I'll post a hacked-up patch (against your patch applied to my tree, minus the hunks that conflict as of today) to illustrate all the places.  But if you want to work through this separately -- and I think *someone* should besides me, so that there's more understanding of rvalue references and moving amongst Gecko hackers -- this'll give a little bit of a cheat sheet.

Your patch plus the delta patch pass jstests, but honestly I'm not all that confident in every change's correctness.  I wonder if it might not be better to try doing this incrementally, or something.  I dunno, this is all head-hurty and I need to go home and eat dinner (!) and sleep before it's time to return to the office today.  Bleh.  Zzz.

::: js/public/HashTable.h
@@ +674,4 @@
>      HashNumber getKeyHash() const     { return keyHash & ~sCollisionBit; }
>  
>      template <class U>
> +    void setLive(HashNumber hn, U &&u)

The basic problem with this part is this: whenever you see |U&& u| in an arguments list, you have to know that *|u| is not an rvalue reference when you use it*.  Passing along |u| to any nested calls inside that method, etc. *will not treat |u| as an rvalue reference.*  Instead it'll treat it as an lvalue reference, because the thing you're passing has a name.  See:

http://thbecker.net/articles/rvalue_references/section_05.html#no_name

Thus, here, you're passing along a |const HashTableEntry&| rather than a |const HashTableEntry&&|, and you invoke the deleted constructor.

@@ +1169,4 @@
>          for (Entry *src = oldTable, *end = src + oldCap; src < end; ++src) {
>              if (src->isLive()) {
>                  HashNumber hn = src->getKeyHash();
> +                findFreeEntry(hn).setLive(hn, mozilla::Forward<T>(src->get()));

I wasn't clear about what "here" meant, wrt mozilla::Forward.  This spot should remain mozilla::Move.  You're taking a reference to a value, and you're converting it to an rvalue reference to indicate that the value should be moved out of the |src->get()| reference, and that reference should afterward be destructible.

Where I meant for you to use mozilla::Forward, was inside |setLive()|.  There, because |u| decayed to a |const HashTableEntry&| lvalue reference, making no changes means |const HashTableEntry&| gets passed.  To perfectly forward the exact type passed to |setLive()| -- that is, an rvalue reference if one was passed in, and an lvalue reference if one was not -- you should use mozilla::Forward inside |setLive()|, like so:

    template <class U>
    void setLive(HashNumber hn, U&& u)
    {
        JS_ASSERT(!isLive());
        keyHash = hn;
        new(mem.addr()) T(mozilla::Forward<U>(u));
        JS_ASSERT(isLive());
    }

::: mfbt/Move.h
@@ +125,5 @@
> +inline T&&
> +Forward(typename RemoveReference<T>::Type& a)
> +{
> +  return static_cast<T&&>(a);
> +}

This isn't a correct implementation of Forward.  (It was according to that link, but I think that link might be slightly dated.  The C++11 N3337 draft has the most accurate semantics, also verifiable against <bits/move.h> if you're using libstdc++.)  You need one version of Forward to handle lvalue references (the one you provided), and you need one version of Forward to handle rvalue references.  This overload would handle the passing of a value by lvalue reference:

   int i;
   Forward<int>(i); // Forward(int&), returns int&

But it won't handle the passing of a value by rvalue reference:

   int f() { return 42; }
   Forward<int>(f()); // f() is an rvalue, won't bind to an lvalue reference

The overload would look like so:

  template<typename T>
  inline T&&
  Forward(typename RemoveReference<T>::Type&& a)
  {
    return static_cast<T&&>(a);
  }

If you look at C++11, it imposes a further requirement beyond this: "if the [rvalue] form is instantiated with an lvalue reference type, the program is ill-formed."  For example:

  int f() { static int x = 99; return x; }
  Forward<int&>(f());

I *think* this case is mostly to prevent deliberate obtuseness, as the usual idiom is not to pass an exact type as parameter to Forward, but to pass a template parameter, corresponding to an argument, with that argument as the argument to Forward.  But I'm not sure.  In any case, this is easily solved by adding a static_assert to get the true additional overload:

  template<typename T>
  inline T&&
  Forward(typename std::remove_reference<T>::type&& a)
  {
    static_assert(!std::is_lvalue_reference<T>::value,
                  "T can't be an lvalue reference");
    return static_cast<T&&>(a);
  }
Luke, probably somebody else should be looking through this stuff in addition to me -- I carpet-bombed HashTable and Vector with move semantics to make things compile, and I'm not necessarily sure this is exactly the way we want to do all of it.  Two things come to mind in particular.

First, the changes to moveConstruct in the delta patch:

    /*
     * Move-constructs objects in the uninitialized range
     * [dst, dst+(srcend-srcbeg)) from the range [srcbeg, srcend).
     */
    template<typename U>
    static inline void moveConstruct(T* dst, U* srcbeg, U* srcend) {
      for (U* p = srcbeg; p < srcend; ++p, ++dst)
        new(dst) T(Move(*p));
    }

which used to take |const U*|, but changed to just |U*| to get the move-ref signature right.  Maybe I should be passing |Move(const_cast<U>(*p))| or something rather than getting rid of the const?  I dunno, I haven't the energy to test if that'd work right now.

Seocnd, I had to make |HashMapEntry::key| non-const so that

    template<typename KeyInput, typename ValueInput>
    bool add(AddPtr &p, KeyInput&& k, ValueInput&& v) {
        Entry e(mozilla::Move(k), mozilla::Move(v));
        return impl.add(p, mozilla::Move(e));
    }

wouldn't cause HashMapEntry(HashMapEntry&&) to initialize its |key| field with a |const Key&&|, decaying to |const Key&| and thus invoking an implicitly-deleted constructor (deleted because HashMapEntry now, post-patches, has a move constructor.  It seems to me there's something seriously wrong about having movable constified fields inside move-constructible things.  But maybe that's just the hour of the day and my coherency speaking right now.

Okay, enough nattering.  Good bye, good night, zzz.
Flags: needinfo?(luke)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> which used to take |const U*|, but changed to just |U*| to get the move-ref
> signature right.

No, you're right, U* makes sense.

> Seocnd, I had to make |HashMapEntry::key| non-const so that

I'd *really* like to have 'key' be a const because it's critically important for the correctness of the hash table.  Really, 4 years ago, I should have just made key/value be private (non-const) members and then just have a key() accessor returning a const Key&.  You could change that if you'd like :)  Otherwise, I think you should leave key const and use a const_cast.
Flags: needinfo?(luke)
OK, I think by this time it's clear that it's not possible for anybody to mechanically perform this conversion.  I think that it's best to add a temporary mozilla::StdMove or something to use C++11 rvalue references, and start converting consumers incrementally, and then once there are no remaining mozilla::Move consumers, remove it and rename StdMove to Move.

That being said, I won't be the right person to work on this, since I don't know anything about the semantics of the code that needs to get modified.
Assignee: ehsan → nobody
Comment on attachment 784856 [details] [diff] [review]
Delta to illustrate the remaining places (that are actually instantiated, yay templates and ticking time bombs) needing fixes

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

Overall the patch looks good.  I'd rather avoid mixing associate-with-T "foo(T&& t)" with associate-with-name "foo(T &t)" in a single file though; all one or the other and this is in js/public, so let's keep it associate-with-name.

Man, && rules are weird; I mean, I think they're the right ones when you see the examples, but weird.  The "no-name" rval reference rule is annoying compared to MoveRef<T>.  I will admit though that I have had a bug or two where I passed a (named) MoveRef<T> to a function (which pillaged the referrent) and then accidentally used the the MoveRef<T> after.

::: js/public/Utility.h
@@ +254,5 @@
>  \
>      template <class T, class P1>\
> +    QUALIFIERS T *NEWNAME(P1&& p1) {\
> +        JS_NEW_BODY(ALLOCATOR, T,\
> +                    (mozilla::Forward<P1>(p1)))\

With these changes, I bet there are a quite a few new_() callsites that, for lack of perfect-forwarding, had to be grossed up that we could now un-gross.

::: js/src/vm/Interpreter.cpp
@@ -1296,4 @@
>  
>  #define CHECK_PCCOUNT_INTERRUPTS() JS_ASSERT_IF(script->hasScriptCounts, switchMask == -1)
>  
> -    register int switchMask = 0;

But now our interpreter will be slow!!!!!
(In reply to comment #11)
> ::: js/src/vm/Interpreter.cpp
> @@ -1296,4 @@
> >  
> >  #define CHECK_PCCOUNT_INTERRUPTS() JS_ASSERT_IF(script->hasScriptCounts, switchMask == -1)
> >  
> > -    register int switchMask = 0;
> 
> But now our interpreter will be slow!!!!!

I think we should make up for that by renaming the variable to something shorter to make things fast again, like sm, or foo even.
I have patches renaming mozilla::Move to mozilla::MozMove.  I didn't want to go with something longer (e.g. DeprecatedMove()) because then we'd have to deal with re-wrapping lines...
I almost have a patch that completes the conversion to true C++11 rvalue references. The discussion of forwarding at http://thbecker.net/articles/rvalue_references/section_01.html was very helpful; I now realize that perfect forwarding was the missing piece I needed to finish the patch.

I have some confusing error messages which I'll dump on y'all in IRC.
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #12)
> I think we should make up for that by renaming the variable to something
> shorter to make things fast again, like sm, or foo even.

For some reason, this reminds me of when WebKit(?) rewrote their JS interpreter in assembly language and doubled their performance. We C++ programmers shouldn't get too snotty.

(I assume their interpreter was competitive to begin with...)
Blocks: 915555
Hi JimB, what's the state of your WIP patch? I started working on converting Vector to rvalue references (as a prerequisite for bug 932865) without realizing you're doing the same thing.

Here's my patch. It only deals with Vector and, in addition to having rvalue references, makes Vector::append more strict about its argument type, which uncovered a few potential issues in the JS code. For example, we are converting narrow string literals to wide string literals at runtime, though we could be using wide string literals from the start; previously Vector::append took both jschar and char so the problem wasn't apparent.

Let me know if you think your patch will replace mine or if I should get mine landed first. Thanks!
(In reply to Jim Chen [:jchen :nchen] from comment #17)
> Created attachment 826983 [details] [diff] [review]
> Make mozilla::Vector use strict types and rvalue references
> 
> Hi JimB, what's the state of your WIP patch? I started working on converting
> Vector to rvalue references (as a prerequisite for bug 932865) without
> realizing you're doing the same thing.

Wow, we're really piling up here; that's too bad.

My patch is almost ready, I hope. It does a complete conversion to the new style; MoveRef and OldMove get deleted. So it handle the hash tables as well.

> Here's my patch. It only deals with Vector and, in addition to having rvalue
> references, makes Vector::append more strict about its argument type, which
> uncovered a few potential issues in the JS code. For example, we are
> converting narrow string literals to wide string literals at runtime, though
> we could be using wide string literals from the start; previously
> Vector::append took both jschar and char so the problem wasn't apparent.

Oh, that's interesting. Where's the place that's converting between the string literal sizes?

(I'm not sure exactly what you mean by "wide string literals". We can't use L"foo" in SpiderMonkey, because wchar_t and jschar are not the same type. And we don't have the NS string constant macros available to us.)

Since my patch is more thorough, why don't we go with that? I'll try to get it landed today or tomorrow.
Putting all my cards on the table. This patch:

- replaces all uses of the old MoveRef and OldMove in js/src with C++11 rvalue references, and appropriate uses of mozilla::Move and mozilla::Forward

- fully modernizes mozilla::Vector, js::Vector, mozilla::HashMap and mozilla::HashSet

well enough that AsmJS.o can compile (AsmJS.o being a particularly heavy user of our container templates with moves). Unless something surprising comes up, I'll do the rest of js/src, and then the rest of the tree, tomorrow.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
That patch also changes Vector to use move semantics in Vector::insert; that seems like it should always have done so.
Comment on attachment 827840 [details] [diff] [review]
Work-in-progress C++11 rvalue reference / Move / Forward conversion

Jeff, how do the HashTable.h and Vector.h (both mfbt/ and js/public/) changes look? There are kludges in the other files, but the files I named I actually think are the way they're supposed to be. I tried to make every reasonable function support moves.

Thanks for pointing me at the discussion earlier in the patch. I've removed some 'const' qualifiers where they don't make sense, as Luke said was all right, and used a const_cast for the key move.
Attachment #827840 - Flags: feedback?(jwalden+bmo)
Nice job!
I've run jit-tests before and after this patch under valgrind memcheck, and there is no change. I've also updated the comments in Move.h, and added an explanation of Forward.
Attachment #827840 - Attachment is obsolete: true
Attachment #827840 - Flags: feedback?(jwalden+bmo)
Attachment #828353 - Flags: feedback?(jwalden+bmo)
Waldo's been working on the new_ method definitions in bug 921561. I haven't done that in this patch, and kludged around it instead, so I'm marking that as a blocker for this.
Depends on: 921561
No longer depends on: 909977
Depends on: 909977
Note that this patch doesn't depend on everything in bug 921561, only the new_ forwarding bit; that's attachment 828749 [details] [diff] [review].
(In reply to Jim Blandy :jimb from comment #18)
> (In reply to Jim Chen [:jchen :nchen] from comment #17)
> > Here's my patch. It only deals with Vector and, in addition to having rvalue
> > references, makes Vector::append more strict about its argument type, which
> > uncovered a few potential issues in the JS code. For example, we are
> > converting narrow string literals to wide string literals at runtime, though
> > we could be using wide string literals from the start; previously
> > Vector::append took both jschar and char so the problem wasn't apparent.
> 
> Oh, that's interesting. Where's the place that's converting between the
> string literal sizes?

In BooleanToStringBuffer [1] for example, when we call |sb->append("true")|, it eventually calls VectorImpl::copyConstruct with the instantiation,

> void copyConstruct(jschar* dst, const char* srcbeg, const char* srcend)
> {
>   for (const char* p = srcbeg; p < srcend; ++p, ++dst)
>     *dst = *p;
> }

So each character is being converted from char to jschar. If we use a strict jschar literal, it's a straight-forward 1-on-1 copy and can be better optimized.

[1] http://mxr.mozilla.org/mozilla-central/source/js/src/vm/StringBuffer.h?rev=bcf130e126aa#144
[2] http://mxr.mozilla.org/mozilla-central/source/mfbt/Vector.h?rev=9ba162545a93#151

> (I'm not sure exactly what you mean by "wide string literals". We can't use
> L"foo" in SpiderMonkey, because wchar_t and jschar are not the same type.
> And we don't have the NS string constant macros available to us.)

Right, I meant char16_t literals. Char16.h includes a MOZ_UTF16 macro (which uses C++11 u"foo" if available) that returns a char16_t literal [3].

[3] http://mxr.mozilla.org/mozilla-central/source/mfbt/Char16.h?rev=97d51d6ae2b7#78

Your patch looks good though. Thanks!
That try push looks solid to me, so I'll request review.
Attachment #828353 - Flags: feedback?(jwalden+bmo) → review?(jwalden+bmo)
Comment on attachment 828353 [details] [diff] [review]
Convert all uses of OldMove and MoveRef to true rvalue references and the modern Move and Forward.

Adding Luke as a reviewer, since a lot of the hairiest stuff is in Asm.js, and also for the C++ and mfbt container expertise.
Attachment #828353 - Flags: review?(luke)
Comment on attachment 828353 [details] [diff] [review]
Convert all uses of OldMove and MoveRef to true rvalue references and the modern Move and Forward.

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

Do you suppose you could put:

 using mozilla::Move;
 using mozilla::Forward

at class scope (so it doesn't leak) in Vector/HashTable to avoid all the mozilla:: noise everywhere?

::: js/src/ds/LifoAlloc.h
@@ +402,5 @@
>  
>      JS_DECLARE_NEW_METHODS(new_, alloc, JS_ALWAYS_INLINE)
>  
> +    template <class T, class P1, class P2, class P3>
> +    JS_ALWAYS_INLINE T *moveToNew(P1 &&p1, P2 &&p2, P3 &&p3) {

Is this necessary?  Seems like JS_DECLARE_NEW_METHODS would do perfect forwarding.

::: js/src/shell/jsheaptools.cpp
@@ +72,5 @@
>           * Move constructor and move assignment. These allow us to store our
>           * incoming edge Vector in the hash table: Vectors support moves, but
>           * not assignments or copy construction.
>           */
> +        Node(Node&& rhs)

Node &&rhs.  Ditto a few times below.
Attachment #828353 - Flags: review?(luke) → review+
I thought of that, but you can't use using-declarations within a class, at least with G++ 4.8.2. With this patch (and associated changes):

--- a/js/public/HashTable.h
+++ b/js/public/HashTable.h
@@ -50,16 +50,19 @@ namespace detail {
 //   called by HashMap must not call back into the same HashMap object.
 // - Due to the lack of exception handling, the user must call |init()|.
 template <class Key,
           class Value,
           class HashPolicy = DefaultHasher<Key>,
           class AllocPolicy = TempAllocPolicy>
 class HashMap
 {
+    using mozilla::Move;
+    using mozilla::Forward;
+
     typedef HashMapEntry<Key, Value> TableEntry;
 
     struct MapHashPolicy : HashPolicy
     {
         typedef Key KeyType;
         static const Key &getKey(TableEntry &e) { return e.key; }
         static void setKey(TableEntry &e, Key &k) { HashPolicy::rekey(const_cast<Key &>(e.key), k); }
     };

I get this error message:

dist/include/js/HashTable.h:58:20: error: using-declaration for non-member at class scope
     using mozilla::Move;
                    ^
(In reply to Luke Wagner [:luke] from comment #30)
> ::: js/src/ds/LifoAlloc.h
> @@ +402,5 @@
> >  
> >      JS_DECLARE_NEW_METHODS(new_, alloc, JS_ALWAYS_INLINE)
> >  
> > +    template <class T, class P1, class P2, class P3>
> > +    JS_ALWAYS_INLINE T *moveToNew(P1 &&p1, P2 &&p2, P3 &&p3) {
> 
> Is this necessary?  Seems like JS_DECLARE_NEW_METHODS would do perfect
> forwarding.

Oh, I removed that in the latest version of the patch, and forgot to refresh.


> ::: js/src/shell/jsheaptools.cpp
> @@ +72,5 @@
> >           * Move constructor and move assignment. These allow us to store our
> >           * incoming edge Vector in the hash table: Vectors support moves, but
> >           * not assignments or copy construction.
> >           */
> > +        Node(Node&& rhs)
> 
> Node &&rhs.  Ditto a few times below.

Fixed; thanks. I got confused by the mix of declarator formatting styles in use across the tree...
(In reply to Jim Blandy :jimb from comment #31)
> I thought of that, but you can't use using-declarations within a class, at
> least with G++ 4.8.2. With this patch (and associated changes):
> 
> --- a/js/public/HashTable.h
> +++ b/js/public/HashTable.h
> @@ -50,16 +50,19 @@ namespace detail {
>  //   called by HashMap must not call back into the same HashMap object.
>  // - Due to the lack of exception handling, the user must call |init()|.
>  template <class Key,
>            class Value,
>            class HashPolicy = DefaultHasher<Key>,
>            class AllocPolicy = TempAllocPolicy>
>  class HashMap
>  {
> +    using mozilla::Move;
> +    using mozilla::Forward;

Perhaps with 'typedef mozilla::Move Move;' instead
(In reply to comment #33)
> (In reply to Jim Blandy :jimb from comment #31)
> > I thought of that, but you can't use using-declarations within a class, at
> > least with G++ 4.8.2. With this patch (and associated changes):
> > 
> > --- a/js/public/HashTable.h
> > +++ b/js/public/HashTable.h
> > @@ -50,16 +50,19 @@ namespace detail {
> >  //   called by HashMap must not call back into the same HashMap object.
> >  // - Due to the lack of exception handling, the user must call |init()|.
> >  template <class Key,
> >            class Value,
> >            class HashPolicy = DefaultHasher<Key>,
> >            class AllocPolicy = TempAllocPolicy>
> >  class HashMap
> >  {
> > +    using mozilla::Move;
> > +    using mozilla::Forward;
> 
> Perhaps with 'typedef mozilla::Move Move;' instead

Move is not a type, it's a function name.

You can't do this at class scope in C++.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #34)
> You can't do this at class scope in C++.

:(
Updated per review comments; carrying forward r=luke
Attachment #828353 - Attachment is obsolete: true
Attachment #828353 - Flags: review?(jwalden+bmo)
Attachment #830397 - Flags: review?(jwalden+bmo)
Comment on attachment 830397 [details] [diff] [review]
Convert all uses of OldMove and MoveRef to true rvalue references and the modern Move and Forward. r=luke

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

So, r+ respecting comments for all but the HashTable.h changes, with followup bugs and followup patches for the smaller issues noted in passing that shouldn't hold up this landing.  Unfortunately I can't record r+ and r- both on a patch, to precisely describe exactly my opinion here.  :-\  I guess clearing is at least not explicitly lying in saying that the patch is overall r+, or something.  Blech.

But regarding the HashTable.h bits, those need a bunch of work, for one, and for two, we really really really need to move that into a separate patch if at all possible, so as not to hold up the rest of this bug.

Pretty sure at this point that I've spent multiple days learning about, reading up on, and reviewing patches involving C++11 rvalue references and move constructors/operators.  My head hurts.  :-)

::: js/public/HashTable.h
@@ +150,5 @@
>  
>      template<typename KeyInput, typename ValueInput>
> +    bool relookupOrAdd(AddPtr &p, KeyInput &&k, ValueInput &&v) {
> +        Entry e(mozilla::Forward<KeyInput>(k), mozilla::Forward<ValueInput>(v));
> +        return impl.relookupOrAdd(p, k, mozilla::Move(e));

Okay, so this is kind of a mess here.

Pre-patch we'd create an Entry using k/v, which were references.  That would copy-construct k and v into the Entry.  Then we'd use a reference to k -- a valid copy of the key in the Entry -- to do the impl.relookupOrAdd.

Post-patch, if someone passes in a temporary |k|, it'll be move-constructed into |e.key|.  That'll leave |k| in unspecified destructible state.  *Using* it after that for the relookupOrAdd, is wrong.

So where does |k| live, to pass into |impl.relookupOrAdd|?  At call time, you could pass |e.key|.  But what if the moved |e| gets moved before |e.key| is used?

It's not actually moved, of course:

    template <class U>
    bool relookupOrAdd(AddPtr& p, const Lookup &l, const U &u)
    {
        p.mutationCount = mutationCount;
        {
            mozilla::ReentrancyGuard g(*this);
            p.entry_ = &lookup(l, p.keyHash, sCollisionBit);
        }
        return p.found() || add(p, u);
    }

So I think we can pass along |e.key| without offending the bad-key gods.  But this is an incredibly horrible bit of fragility!  Please file a bug so that we can get rid of it, um, somehow.  (I'm not sure what that way can be, but maybe someone else will have a bright idea here on how to get rid of it.)

Another thing we should do -- make relookupOrAdd and such assert that the key/lookup passed in are consistent -- that the hash of |l| is equal to |p.keyHash|.  That would detect mistakes wherein something gets moved around and hashes and keys get out of sync.  Another bug?

@@ +224,1 @@
>          AddPtr p = lookupForAdd(k);

mozilla::Forward<KeyInput>(k), if we're going to avoid copying.

@@ +227,3 @@
>              return true;
>          }
> +        return add(p, mozilla::Forward <KeyInput>(k), mozilla::Forward<ValueInput>(v));

Extraneous space here.

...and, ugh.  Now we need |k| again, but we just copied it into the AddPtr.  And I think it's supposed to stay in there, so we can't just move it back out again or something like that.  So, uh, mess.

I'll keep plodding through this file, but I think there's enough even now to say that we should do hash table moving improvements in a separate patch.  Is there any chance at all you can disentangle the HashTable.h changes from the rest of the changes, and land those all together as a single unit?  HashTable.h clearly needs non-trivial refactoring, in various parts, for it to implement key/value movability correctly.  And neither of us really wants to hold up all the other largely-fine changes in this patch, just for the hard parts in this one file.

@@ +232,5 @@
>      // Like put, but assert that the given key is not already present.
>      template<typename KeyInput, typename ValueInput>
> +    bool putNew(KeyInput &&k, ValueInput &&v) {
> +        Entry e(mozilla::Forward<KeyInput>(k), mozilla::Forward<ValueInput>(v));
> +        return impl.putNew(e.key, mozilla::Move(e));

Again it looks very-fragilely safe to pass in |e.key| even with a Move(e) here, because |Move(e)| is only used after |e.key| is fully used (only to generate a hash, it looks like).  But surely we can we do something, anything, better for this!

@@ +379,3 @@
>  
> +    template <typename U>
> +    bool relookupOrAdd(AddPtr &p, const Lookup &l, U &&u) {

...a set data structure has APIs that pass in a key and a (presumably exactly the same) value?  That's crazytalk.  But, uh, crazytalk to think about in some other bug.

@@ +452,5 @@
>  
>      // Like put, but assert that the given key is not already present.
> +    template <typename U>
> +    bool putNew(U &&u) {
> +        return impl.putNew(u, mozilla::Forward<U>(u));

This is also just crazy-fragile, wrt the first |u| only being used before the forwarded |u| is consumed.  :-\  More that needs rigorizing in the second round of patching to fix HashTable.h, I think.

@@ +878,2 @@
>      {
> +        mozilla::PodAssign(this, &rhs);

Hmm, so.  I was going to say pass |Move(rhs)| to AllocPolicy, but that's not quite right.  If you move that, you've destroyed that component of |rhs|, and then the |PodAssign| becomes wrong (it's copying from destructible memory, and it's overwriting the |AllocPolicy| component of |this|).  C++ requires you to construct the base class, so you can't just omit that part.  And if you did, what if the |AllocPolicy| part had to go through a move or copy constructor (say, because it had internal pointers or whatever)?

I think all this means we probably have to revert to base principles here: use |Move(rhs)| to initialize the base, then move/copy each individual non-inherited field of |HashTable|.  To do this, I would probably first move all the fields of |HashTable| up to the very top of the class, so that they're all in one single easily-perused location, then probably move the move constructor and assignment operators immediately underneath there, then fill in the guts of this method as required.  Separate patches for all those bits would be nice, for reader reviewability.

@@ +883,4 @@
>          if (table)
>              destroyTable(*this, table, capacity());
> +        mozilla::PodAssign(this, &rhs);
> +        rhs.table = nullptr;

|new (this) HashTable(Move(rhs));| is more principled, and it'll pick up |AllocPolicy| move support for free, in concert with the previous set of desired changes.

@@ +1210,1 @@
>                  src->destroy();

(This |src->destroy()| is what twigged me to the "safely-destructible" intricacies I asked to be clarified, fwiw.  :-) )

::: js/public/MemoryMetrics.h
@@ +330,1 @@
>          : ZoneStatsPod(other),

mozilla::Move(other), in case that class ever picks up anything substantial.

::: js/src/builtin/MapObject.cpp
@@ +167,5 @@
>       * not. On allocation failure, return false. If this returns false, it
>       * means the element was not added to the table.
>       */
> +    template <typename ElementInput>
> +    bool put(ElementInput &&element) {

I'd have used |typename U| to follow the usual U-is-T-or-a-lookalike convention, but this is fine.

@@ +592,5 @@
>          for (Data *rp = data; rp != end; rp++) {
>              if (!Ops::isEmpty(Ops::getKey(rp->element))) {
>                  HashNumber h = prepareHash(Ops::getKey(rp->element)) >> hashShift;
>                  if (rp != wp)
> +                    wp->element = Move(rp->element);

Wrt self-move-assignment as discussed in Move.h, you can see how we explicitly don't have to handle it here -- this seems the common sort of case to me.  :-)

@@ +692,2 @@
>  
>          const Key key;

File the followup to make |key| in this file, and in HashTable.h, non-const?  We should do that, definitely, if not holding the rest of this up.

::: js/src/jit/AsmJS.cpp
@@ +633,5 @@
>        : argTypes_(cx) {}
>      Signature(ExclusiveContext *cx, RetType retType)
>        : argTypes_(cx), retType_(retType) {}
> +    Signature(VarTypeVector &&argTypes, RetType retType)
> +      : argTypes_(Move(argTypes)), retType_(retType) {}

Given that RetType is a class, I'd slightly prefer Move() on it just in case it ever picks up anything substantial.

@@ +635,5 @@
>        : argTypes_(cx), retType_(retType) {}
> +    Signature(VarTypeVector &&argTypes, RetType retType)
> +      : argTypes_(Move(argTypes)), retType_(retType) {}
> +    Signature(Signature &&rhs)
> +      : argTypes_(Move(rhs.argTypes_)), retType_(rhs.retType_) {}

Move() again, same reason.

::: js/src/vm/MemoryMetrics.cpp
@@ +137,1 @@
>      : StringInfo(info)

Should be two-space-indented.  Also I'd slightly prefer Move() in case StringInfo ever picks up anything substantial.

@@ +144,3 @@
>  {
>      this->~NotableStringInfo();
> +    new (this) NotableStringInfo(mozilla::Move(info));

I'd put a |using mozilla::Move;| at top to get rid of the prefixes on all these, myself.

::: mfbt/Move.h
@@ +54,5 @@
>   *
> + * If a constructor has a single argument of type 'T&&' (an 'rvalue reference
> + * to T'), that indicates that it is a 'move constructor'. That's 1). It should
> + * move, not copy, its argument into the object being constructed. It may leave
> + * the original in any safely-destructible state.

I learned, and/or realized, something reading this patch, then doing other research: safely-destructible doesn't mean anything like "all resources have been freed" or anything.  It's perfectly acceptable to have this as a move constructor:

  C(const C& c) { ...copy-construct however, not touching c... }
  C(C&& c)
  {
    new (this) C(c);
  }

leaving |c| exactly as it was before.  It's not very useful, probably.  But it *is* valid to do.  So any moved-from object still has to have its destructor called at some point, or else there's the *potential* to leak any resources a moved-from object still contains.  I think this deserves clarification here.  "safely-destructible" does not imply that all associated resources have been relinquished -- you have to ensure the destructor is called afterward, in some way, for fully acceptable behavior.

I would wordsmith something up for this, but it's very very nearly dinner time.  Propose something in a second patch atop the parts of this you can land, without the HashTable.h changes, and r?me on it?  Or I can still write something, but it's not happening tonight.  :-)

@@ +59,2 @@
>   *
> + * If a constructor's argument is an rvalue, as in 'C(f(x))' or 'C(x+y)', as

C(x + y), for readability

@@ +92,5 @@
>   * A class with a move constructor can also provide a move assignment operator,
>   * which runs this's destructor, and then applies the move constructor to
>   * *this's memory. A typical definition:
>   *
>   *   C& operator=(C&& rhs) {  // or |MoveRef<C> rhs|

Kill off the MoveRef mention here.

@@ +94,5 @@
>   * *this's memory. A typical definition:
>   *
>   *   C& operator=(C&& rhs) {  // or |MoveRef<C> rhs|
>   *     this->~C();
>   *     new(this) C(rhs);

new(this) C(Move(rhs))

@@ +105,3 @@
>   *
>   * This destroys c1, moves c1's value to c2, and leaves c1 in an undefined but
>   * destructible state.

As far as I can tell, considered opinions differ as to whether move assignment operators should, or should not, deal with self-moves.  (See, for example, the considerable discussion in <http://stackoverflow.com/a/9322542/115698> -- and that's only a single person's opinion, albeit playing devil's advocate with a few other opinions.  :-) )  It's fairly difficult for it to actually happen, unintentionally, so I'm inclined to say we should write code assuming it can't happen.  But it might be worth adding a

  MOZ_ASSERT(&rhs != this, "self-moves are prohibited");

and an extra paragraph like

"""
Technically move assignment has the same potential pitfall as regular assignment, in that self-assignment may need to be handled carefully.  In practice it's going to be fairly hard to do a self-assignment with moves, given that rvalue references only show up if you use Move explicitly or have a temporary that returns one of its inputs.  So it seems reasonable to assert it doesn't happen, then stamp out any places where it could possibly occur if they ever manifest, using tactics that don't require handling move self-assignment.
"""

I'd also say, while we're touching all these, we should add that assert to every move assignment operator.  Or you can do that as a followup, either's fine with me.  But we should do that soon, to detect any errors, no matter how weird/impossible the case might be.

@@ +135,5 @@
> + *
> + *      X foo();
> + *      Y yy;
> + *
> + *      C(foo(), yy)

I'd add make that |foo(int)| and |foo(5)| so that the reader isn't tripped up initially thinking that was a variable declaration.

@@ +158,5 @@
> + * into 'x', but copy 'yy' into 'y'.
> + *
> + * This header file defines Move and Forward in the mozilla namespace. It's up
> + * to individual containers to annotate moves as such, by calling Move; and it's
> + * up to individual types to define move constructors.

Maybe add " and move assignment operators as desired"?

@@ +178,5 @@
>    return static_cast<typename RemoveReference<T>::Type&&>(a);
>  }
>  
>  /**
> + * These two overloads are identical to std::Forward(); they are necessary until

std::forward

::: mfbt/Vector.h
@@ +85,5 @@
>       */
>      template<typename U>
> +    static inline void moveConstruct(T* dst, U* srcbeg, U* srcend) {
> +      for (U* p = srcbeg; p < srcend; ++p, ++dst)
> +        new(dst) T(Move(*p));

You don't need Move() here, because *p is already a temporary, ergo an rvalue reference.  (Having it will actually create warnings with sufficiently old gcc, if bug 915555 is any guide.)

@@ +114,5 @@
>          return false;
>        T* dst = newbuf;
>        T* src = v.beginNoCheck();
>        for (; src < v.endNoCheck(); ++dst, ++src)
> +        new(dst) T(Move(*src));

This doesn't need Move() around it, either, same reason.

@@ +314,5 @@
>      typedef T ElementType;
>  
>      VectorBase(AllocPolicy = AllocPolicy());
> +    VectorBase(ThisVector&&); /* Move constructor. */
> +    ThisVector& operator=(ThisVector&&); /* Move assignment. */

We should probably

  VectorBase(VectorBase&&) MOZ_DELETE;
  void operator=(VectorBase&&) MOZ_DELETE;

as well at the end of the class, to be safe.

@@ +477,1 @@
>        internalAppend(u);

Move(u), else this won't move.

@@ +584,1 @@
>    : AllocPolicy(rhs)

Move(rhs)

@@ +612,2 @@
>  #ifdef DEBUG
> +    rhs.mReserved = sInlineCapacity;

Hmm.  This was supposed to be zero to start, back in the day -- doing it this way exposes the internal inline capacity, which is supposed to be an implementation detail (the N you pass in isn't always trusted, when N is large).  I guess we should fix this in some other bug.
Attachment #830397 - Flags: review?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37)
> But regarding the HashTable.h bits, those need a bunch of work, for one, and
> for two, we really really really need to move that into a separate patch if
> at all possible, so as not to hold up the rest of this bug.

When I started out on this work, I tried to do the conversion on a per-type basis, and I got very confused. I think we can fix up the problems quickly.


> So I think we can pass along |e.key| without offending the bad-key gods. 
> But this is an incredibly horrible bit of fragility!  Please file a bug so
> that we can get rid of it, um, somehow.  (I'm not sure what that way can be,
> but maybe someone else will have a bright idea here on how to get rid of it.)

I'm very glad you caught this. I'll try to be more careful in the future.

Since HashTable is meant only as an implementation detail for HashMap and HashSet, and not for general public use, I think it's not too terrible if its interfaces are locally delicate. I've used |e.key| in the appropriate place, and added a comment to HashTable::relookupOrAdd pointing out that |l| might be a reference to a piece of |u|. I've added an assertion as well:

  JS_ASSERT(prepareHash(l) == p.keyHash);

because what's stored in an AddPtr isn't simply a hash number as returned by the policy.


> @@ +224,1 @@
> >          AddPtr p = lookupForAdd(k);
> 
> mozilla::Forward<KeyInput>(k), if we're going to avoid copying.
> 
> @@ +227,3 @@
> >              return true;
> >          }
> > +        return add(p, mozilla::Forward <KeyInput>(k), mozilla::Forward<ValueInput>(v));
> 
> Extraneous space here.
> 
> ...and, ugh.  Now we need |k| again, but we just copied it into the AddPtr. 
> And I think it's supposed to stay in there, so we can't just move it back
> out again or something like that.  So, uh, mess.

I think you're misunderstanding here: neither Ptr nor AddPtr hold references to the key. So the key is still used in a 'linear' fashion: it doesn't need to get duplicated into the AddPtr in HashMap::put.


> @@ +878,2 @@
> >      {
> > +        mozilla::PodAssign(this, &rhs);
> 
> Hmm, so.  I was going to say pass |Move(rhs)| to AllocPolicy, but that's not
> quite right.  If you move that, you've destroyed that component of |rhs|,
> and then the |PodAssign| becomes wrong (it's copying from destructible
> memory, and it's overwriting the |AllocPolicy| component of |this|).  C++
> requires you to construct the base class, so you can't just omit that part. 
> And if you did, what if the |AllocPolicy| part had to go through a move or
> copy constructor (say, because it had internal pointers or whatever)?
> 
> I think all this means we probably have to revert to base principles here:
> use |Move(rhs)| to initialize the base, then move/copy each individual
> non-inherited field of |HashTable|.  To do this, I would probably first move
> all the fields of |HashTable| up to the very top of the class, so that
> they're all in one single easily-perused location, then probably move the
> move constructor and assignment operators immediately underneath there, then
> fill in the guts of this method as required.  Separate patches for all those
> bits would be nice, for reader reviewability.

This is a pre-existing problem, though. The PodAssign is just wrong, since it makes the unjustified assumption that AllocPolicy can be PodAssigned. I think it should be filed as a separate bug.


> @@ +883,4 @@
> >          if (table)
> >              destroyTable(*this, table, capacity());
> > +        mozilla::PodAssign(this, &rhs);
> > +        rhs.table = nullptr;
> 
> |new (this) HashTable(Move(rhs));| is more principled, and it'll pick up
> |AllocPolicy| move support for free, in concert with the previous set of
> desired changes.

I agree, but again, since this patch doesn't change the behavior of that code, I think fixing this problem should be a separate patch.


> ::: js/public/MemoryMetrics.h
> @@ +330,1 @@
> >          : ZoneStatsPod(other),
> 
> mozilla::Move(other), in case that class ever picks up anything substantial.

Fixed, thanks.


> @@ +692,2 @@
> >  
> >          const Key key;
> 
> File the followup to make |key| in this file, and in HashTable.h, non-const?
> We should do that, definitely, if not holding the rest of this up.

I guess I agree with Luke's argument in comment 9 that key should remain const, and cast away the const in those exceptional cases where we know it's okay. (Perhaps I'm misunderstanding what you're requesting here?)


> ::: js/src/jit/AsmJS.cpp
> @@ +633,5 @@
> >        : argTypes_(cx) {}
> >      Signature(ExclusiveContext *cx, RetType retType)
> >        : argTypes_(cx), retType_(retType) {}
> > +    Signature(VarTypeVector &&argTypes, RetType retType)
> > +      : argTypes_(Move(argTypes)), retType_(retType) {}
> 
> Given that RetType is a class, I'd slightly prefer Move() on it just in case
> it ever picks up anything substantial.

Done.


> @@ +635,5 @@
> >        : argTypes_(cx), retType_(retType) {}
> > +    Signature(VarTypeVector &&argTypes, RetType retType)
> > +      : argTypes_(Move(argTypes)), retType_(retType) {}
> > +    Signature(Signature &&rhs)
> > +      : argTypes_(Move(rhs.argTypes_)), retType_(rhs.retType_) {}
> 
> Move() again, same reason.

Done.


> ::: js/src/vm/MemoryMetrics.cpp
> @@ +137,1 @@
> >      : StringInfo(info)
> 
> Should be two-space-indented.  Also I'd slightly prefer Move() in case
> StringInfo ever picks up anything substantial.

Done.


> @@ +144,3 @@
> >  {
> >      this->~NotableStringInfo();
> > +    new (this) NotableStringInfo(mozilla::Move(info));
> 
> I'd put a |using mozilla::Move;| at top to get rid of the prefixes on all
> these, myself.

Done.


> ::: mfbt/Move.h
> @@ +54,5 @@
> >   *
> > + * If a constructor has a single argument of type 'T&&' (an 'rvalue reference
> > + * to T'), that indicates that it is a 'move constructor'. That's 1). It should
> > + * move, not copy, its argument into the object being constructed. It may leave
> > + * the original in any safely-destructible state.
> 
> I learned, and/or realized, something reading this patch, then doing other
> research: safely-destructible doesn't mean anything like "all resources have
> been freed" or anything.  It's perfectly acceptable to have this as a move
> constructor:
> 
>   C(const C& c) { ...copy-construct however, not touching c... }
>   C(C&& c)
>   {
>     new (this) C(c);
>   }
> 
> leaving |c| exactly as it was before.  It's not very useful, probably.  But
> it *is* valid to do.  So any moved-from object still has to have its
> destructor called at some point, or else there's the *potential* to leak any
> resources a moved-from object still contains.  I think this deserves
> clarification here.  "safely-destructible" does not imply that all
> associated resources have been relinquished -- you have to ensure the
> destructor is called afterward, in some way, for fully acceptable behavior.

I've added the following paragraph to Move.h:

 * As we say, a move must leave the original in a "destructible" state. The
 * original's destructor will still be called, so if a move doesn't
 * actually steal all its resources, that's fine. We require only that the
 * move destination must take on the original's value; and that destructing
 * the original must not break the move destination.


> @@ +59,2 @@
> >   *
> > + * If a constructor's argument is an rvalue, as in 'C(f(x))' or 'C(x+y)', as
> 
> C(x + y), for readability

Done.


> @@ +92,5 @@
> >   * A class with a move constructor can also provide a move assignment operator,
> >   * which runs this's destructor, and then applies the move constructor to
> >   * *this's memory. A typical definition:
> >   *
> >   *   C& operator=(C&& rhs) {  // or |MoveRef<C> rhs|
> 
> Kill off the MoveRef mention here.

Rats. Thanks.


> @@ +94,5 @@
> >   * *this's memory. A typical definition:
> >   *
> >   *   C& operator=(C&& rhs) {  // or |MoveRef<C> rhs|
> >   *     this->~C();
> >   *     new(this) C(rhs);
> 
> new(this) C(Move(rhs))

Rats! Thanks.


> As far as I can tell, considered opinions differ as to whether move
> assignment operators should, or should not, deal with self-moves.

The role of that comment should be tutorial; this is getting into the weeds a bit. I've changed the definition of the sample move assignment operator, because it's utterly wrong when self-assignment occurs:

 *   C& operator=(C&& rhs) {
 *     MOZ_ASSERT(&rhs != this, "self-moves are prohibited");
 *     this->~C();
 *     new(this) C(Move(rhs));
 *     return *this;
 *   }

and added the following paragraph:

 * (Opinions differ on whether move assignment operators should deal with move
 * assignment of an object onto itself. It seems wise to either handle that
 * case, or assert that it does not occur.)


> I'd also say, while we're touching all these, we should add that assert to
> every move assignment operator.  Or you can do that as a followup, either's
> fine with me.  But we should do that soon, to detect any errors, no matter
> how weird/impossible the case might be.

Done.


> @@ +135,5 @@
> > + *
> > + *      X foo();
> > + *      Y yy;
> > + *
> > + *      C(foo(), yy)
> 
> I'd add make that |foo(int)| and |foo(5)| so that the reader isn't tripped
> up initially thinking that was a variable declaration.

Done.


> @@ +158,5 @@
> > + * into 'x', but copy 'yy' into 'y'.
> > + *
> > + * This header file defines Move and Forward in the mozilla namespace. It's up
> > + * to individual containers to annotate moves as such, by calling Move; and it's
> > + * up to individual types to define move constructors.
> 
> Maybe add " and move assignment operators as desired"?

Amended.


> @@ +178,5 @@
> >    return static_cast<typename RemoveReference<T>::Type&&>(a);
> >  }
> >  
> >  /**
> > + * These two overloads are identical to std::Forward(); they are necessary until
> 
> std::forward

Fixed.


> ::: mfbt/Vector.h
> @@ +85,5 @@
> >       */
> >      template<typename U>
> > +    static inline void moveConstruct(T* dst, U* srcbeg, U* srcend) {
> > +      for (U* p = srcbeg; p < srcend; ++p, ++dst)
> > +        new(dst) T(Move(*p));
> 
> You don't need Move() here, because *p is already a temporary, ergo an
> rvalue reference.  (Having it will actually create warnings with
> sufficiently old gcc, if bug 915555 is any guide.)

No, *p is an lvalue. Without the move, this will invoke T's copy constructor.


> @@ +314,5 @@
> >      typedef T ElementType;
> >  
> >      VectorBase(AllocPolicy = AllocPolicy());
> > +    VectorBase(ThisVector&&); /* Move constructor. */
> > +    ThisVector& operator=(ThisVector&&); /* Move assignment. */
> 
> We should probably
> 
>   VectorBase(VectorBase&&) MOZ_DELETE;
>   void operator=(VectorBase&&) MOZ_DELETE;
> 
> as well at the end of the class, to be safe.

Done.


> @@ +477,1 @@
> >        internalAppend(u);
> 
> Move(u), else this won't move.

You mean Forward<U>(u), right? Done.


> @@ +584,1 @@
> >    : AllocPolicy(rhs)
> 
> Move(rhs)

Done.
Patch, revised per Waldo's comments.
Attachment #830397 - Attachment is obsolete: true
Attachment #831202 - Flags: review?(jwalden+bmo)
Attachment #779237 - Attachment is obsolete: true
Attachment #784446 - Attachment is obsolete: true
Attachment #784856 - Attachment is obsolete: true
Attachment #826983 - Attachment is obsolete: true
So, the assertion the patch adds that checks (or is meant to check) that the AddPtr and Lookup value haven't been ruined by an early move from the entry is failing. I suspect that this is a pre-existing problem, so I'll try a push with that assertion alone.
Here's a try push that just adds the AddPtr-Lookup consistency assertion:
https://tbpl.mozilla.org/?tree=Try&rev=41101fbeb04c
(In reply to Jim Blandy :jimb from comment #43)
> Here's a try push that just adds the AddPtr-Lookup consistency assertion:
> https://tbpl.mozilla.org/?tree=Try&rev=41101fbeb04c

Except it won't tell us anything, because we only run the SM(r ggc) tests when the patch touches js/src --- and this patch only touches js/public. RAGARHUGHUGRR

https://tbpl.mozilla.org/?tree=Try&rev=4c2c0b7dd53b
Luke, here's a patch that adds an assertion I believe should pass. It doesn't, on the enable-root-analysis and generational GC builds; see the Try push above. Am I misunderstanding AddPtrs, or is this a real bug?
Flags: needinfo?(luke)
Comment on attachment 831202 [details] [diff] [review]
Convert all uses of OldMove and MoveRef to true rvalue references and the modern Move and Forward. r=luke

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

::: js/public/HashTable.h
@@ +224,1 @@
>          AddPtr p = lookupForAdd(k);

Ah, yes, lookupForAdd takes a const&, so no chance of an extra copy undesirably manifesting itself.

@@ +1535,1 @@
>          HashPolicy::setKey(t, const_cast<Key &>(k));

Semi-irrelevant aside, but I got fairly confused by there being a template parameter named HashPolicy, and a separate js::detail::HashPolicy struct.  One or the other of these should be renamed, so as not to induce mental shadowing confusion.  Separate bug/patch.

::: js/public/MemoryMetrics.h
@@ +328,5 @@
>  
> +    ZoneStats(ZoneStats &&other)
> +        : ZoneStatsPod(mozilla::Move(other)),
> +          strings(mozilla::Move(other.strings)),
> +          notableStrings(mozilla::Move(other.notableStrings))

ZoneStates(...)
  : ZoneStatsPod(...),
    strings(...)

Two-space indent there.

::: js/src/builtin/MapObject.cpp
@@ +172,2 @@
>          HashNumber h = prepareHash(Ops::getKey(element));
>          if (Data *e = lookup(Ops::getKey(element), h)) {

*looks askance at computing Ops::getKey(element) twice rather than introducing a local*

::: js/src/jit/AsmJSModule.h
@@ +340,5 @@
>  
>          ProfiledBlocksFunction(JSAtom *name, unsigned start, unsigned endInline, unsigned end,
>                                 jit::BasicBlocksVector &blocksVector)
>            : ProfiledFunction(name, start, end), endInlineCodeOffset(endInline),
> +            blocks(mozilla::Move(blocksVector))

Hmm, blocksVector should probably be an rvalue reference if we're going to cast/move it under the hood.  Followup bug if that driveby look is accurate?

@@ +347,5 @@
>          }
>  
>          ProfiledBlocksFunction(const ProfiledBlocksFunction &copy)
>            : ProfiledFunction(copy.name, copy.startCodeOffset, copy.endCodeOffset),
> +            endInlineCodeOffset(copy.endInlineCodeOffset), blocks(mozilla::Move(copy.blocks))

This is kind of strange, having a copy constructor that destroys stuff in the copy-ee.  Seems like something better should be done here, more followup.

::: js/src/vm/MemoryMetrics.cpp
@@ +134,5 @@
>      strcpy(buffer, info.buffer);
>  }
>  
> +NotableStringInfo::NotableStringInfo(NotableStringInfo &&info)
> +    : StringInfo(Move(info))

Only indent two spaces.

@@ +413,5 @@
>          if (info.totalSizeOf() < NotableStringInfo::notableSize() ||
>              !zStats.notableStrings.growBy(1))
>              continue;
>  
> +        zStats.notableStrings.back() = Move(NotableStringInfo(str, info));

Move() not necessary here, NSI(...) is a temporary.
Attachment #831202 - Flags: review?(jwalden+bmo) → review+
> I guess I agree with Luke's argument in comment 9 that key should remain
> const, and cast away the const in those exceptional cases where we know
> it's okay. (Perhaps I'm misunderstanding what you're requesting here?)

I meant the bit about making the key field non-const, and exposing only a public const& accessor, as he noted in that same comment.  :-)  And maybe one public non-const& accessor named specifically to indicate that it's moving stuff around.
(In reply to Jim Blandy :jimb from comment #45)
Yes, it seems like it should pass, but tbh this is not all that fresh in my memory so I could be forgetting something.  However, I don't think I am and I suspect it's a HashTable-usage bug.  GGC does some pretty trixie stuff with HashTables, so probably worth filing a bug on and cc'ing the GGC crew.
Flags: needinfo?(luke)
Filed bug 939993 for the AddPtr misuse.
Blocks: 940008
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #46)
> @@ +1535,1 @@
> >          HashPolicy::setKey(t, const_cast<Key &>(k));
> 
> Semi-irrelevant aside, but I got fairly confused by there being a template
> parameter named HashPolicy, and a separate js::detail::HashPolicy struct. 
> One or the other of these should be renamed, so as not to induce mental
> shadowing confusion.  Separate bug/patch.

I can't find what you're referring to. Where is js::detail::HashPolicy defined?


> ::: js/public/MemoryMetrics.h
> @@ +328,5 @@
> >  
> > +    ZoneStats(ZoneStats &&other)
> > +        : ZoneStatsPod(mozilla::Move(other)),
> > +          strings(mozilla::Move(other.strings)),
> > +          notableStrings(mozilla::Move(other.notableStrings))
> 
> ZoneStates(...)
>   : ZoneStatsPod(...),
>     strings(...)
> 
> Two-space indent there.

Fixed here, along with similar cases in MemoryMetrics.cpp.


> ::: js/src/jit/AsmJSModule.h
> @@ +340,5 @@
> >  
> >          ProfiledBlocksFunction(JSAtom *name, unsigned start, unsigned endInline, unsigned end,
> >                                 jit::BasicBlocksVector &blocksVector)
> >            : ProfiledFunction(name, start, end), endInlineCodeOffset(endInline),
> > +            blocks(mozilla::Move(blocksVector))
> 
> Hmm, blocksVector should probably be an rvalue reference if we're going to
> cast/move it under the hood.  Followup bug if that driveby look is accurate?

Filed as bug 940008.


> @@ +347,5 @@
> >          }
> >  
> >          ProfiledBlocksFunction(const ProfiledBlocksFunction &copy)
> >            : ProfiledFunction(copy.name, copy.startCodeOffset, copy.endCodeOffset),
> > +            endInlineCodeOffset(copy.endInlineCodeOffset), blocks(mozilla::Move(copy.blocks))
> 
> This is kind of strange, having a copy constructor that destroys stuff in
> the copy-ee.  Seems like something better should be done here, more followup.

Noted in the bug above.


> @@ +413,5 @@
> >          if (info.totalSizeOf() < NotableStringInfo::notableSize() ||
> >              !zStats.notableStrings.growBy(1))
> >              continue;
> >  
> > +        zStats.notableStrings.back() = Move(NotableStringInfo(str, info));
> 
> Move() not necessary here, NSI(...) is a temporary.

Ah, thanks. Fixed.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #47)
> > I guess I agree with Luke's argument in comment 9 that key should remain
> > const, and cast away the const in those exceptional cases where we know
> > it's okay. (Perhaps I'm misunderstanding what you're requesting here?)
> 
> I meant the bit about making the key field non-const, and exposing only a
> public const& accessor, as he noted in that same comment.  :-)  And maybe
> one public non-const& accessor named specifically to indicate that it's
> moving stuff around.

Filed as bug 940033.
One last build-only try push, for paranoia:
https://tbpl.mozilla.org/?tree=Try&rev=673bb0827079
Try push looks good; the ggc build failure is due to a lack of disk space while installing the prerequisites for the build. (Um, wow. There's a lot going on on the build machines that I didn't expect.) And GGC builds fine on my machine.

Waiting for the tree to re-open...
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf4e009ba00
Flags: in-testsuite-
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Jim Blandy :jimb from comment #50)
> I can't find what you're referring to. Where is js::detail::HashPolicy
> defined?

Hmm, not seeing it on second look.  No problems on this front, then.
https://hg.mozilla.org/mozilla-central/rev/bbf4e009ba00
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Seeing a compilation failure that appears to be caused by these changes.

This might only be hit due to compiling with --enable-perf

In file included from src/js/src/jsobj.cpp:42:
src/js/src/jit/AsmJSModule.h:352:60: error: call to implicitly-deleted copy constructor of 'jit::BasicBlocksVector' (aka 'Vector<js::jit::Record, 1, js::SystemAllocPolicy>')
            endInlineCodeOffset(copy.endInlineCodeOffset), blocks(mozilla::Move(copy.blocks))
                                                           ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~
../../dist/include/js/Vector.h:58:5: note: copy constructor is implicitly deleted because 'Vector<js::jit::Record, 1, js::SystemAllocPolicy>' has a user-declared move constructor
    Vector(Vector &&vec) : Base(mozilla::Move(vec)) {}
    ^
1 error generated.
Douglas, please file a new bug and make it block this one.
(In reply to :Ms2ger from comment #58)
> Douglas, please file a new bug and make it block this one.

Perhaps it would be addressed in bug 940008 which is already blocking on this one.
Depends on: 941803
(In reply to Douglas Crosher [:dougc] from comment #59)
> (In reply to :Ms2ger from comment #58)
> > Douglas, please file a new bug and make it block this one.
> 
> Perhaps it would be addressed in bug 940008 which is already blocking on
> this one.

Nope, this is a separate problem. Filed as bug 941803.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: