Jeff Walden [:Waldo] Assignee
	Description • 11 years ago

Needed for rolling-mean stuff in bug 888084.  Conceptually straightforward enough, just requires transplanting a bunch of stuff out of js/ into mfbt/ and dealing with a bunch of small little issues.  Patches coming Real Soon Now.

	Jeff Walden [:Waldo] Assignee
	Comment 1 • 11 years ago

Luke would be more ideal for these reviews, but he's out for the week, and I've kind of delayed the rolling mean stuff (dep list) long enough already.  And this is basically extraction-y stuff.

This might want hookup to a default memory reporter, but please let's defer to a followup.  :-)

	Jeff Walden [:Waldo] Assignee
	Comment 7 • 11 years ago

This bit is kind of gnarly, but I added tests, so hopefully it's correct.  :-)  (My first try was crash and burn, so I wrote tests and fixed stuff, now I'm hopeful.  Obviously this whole stack will get some try-servering at some point.)

	Jeff Walden [:Waldo] Assignee
	Comment 8 • 11 years ago

mozilla/Move.h is arguably not quite the right place, but given it uses mozilla::Move it seems not totally unreasonable.

	Jeff Walden [:Waldo] Assignee
	Comment 9 • 11 years ago

And now, a few patches killing extraneous js/ dependencies in various ways.

	Jeff Walden [:Waldo] Assignee
	Comment 10 • 11 years ago

Arguably IsRelocatableMumble should be in a GC header, but please don't scope-creep me bro, it's an easy followup.

	Jeff Walden [:Waldo] Assignee
	Comment 11 • 11 years ago

Sigh, template typedefs would make this so much nicer and eliminate the CRTP, but oh well.  I guess we have a public VectorBase for now, that people (i.e. js::Vector, really) can subclass, and mozilla::Vector is the thing people would generally use.

I mostly tried to keep the interface the same-ish, but because of CRTP issues surrounding append(U) versus append(const VectorBase<U,O,BP,UV>&), I renamed the latter method to appendAll as discussed, for a quick fix.  There's almost certainly magic to throw at that to keep the method the same, but it doesn't really matter much.

AsmJS.cpp needs all the js:: prefixes because it |using namespace js| and |using namespace mozilla| both.  And that header and its .h are messes, dependency-wise, so it's non-trivial fixing that now.  Followup yak-shaving please!

	Jeff Walden [:Waldo] Assignee
	Comment 12 • 11 years ago

Adding luke in case he feels like following any of this at all in his week off, and in case he wants to confirm in permanent form he's fine with moving Vector to mfbt.  :-)  Hopefully he's having too much fun unpacking to do anything to follow this, tho.  :-)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 13 • 11 years ago

Comment on attachment 772463 [details] [diff] [review]
2 - mozilla/Move.h

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

::: mfbt/Move.h
@@ +40,5 @@
> + * are copies, and can't be optimized this way. So we need:
> + *
> + * 1) a way for a particular invocation of a copy constructor to say that it's
> + *    really a move, and that the value of the original isn't important
> + *    afterwards (althought it must still be safe to destroy); and

althought?

@@ +129,5 @@
> +  return MoveRef<T>(t);
> +}
> +
> +template<typename T>
> +inline MoveRef<T> Move(const T& t)

Inconsistent line breaking with the other overload

@@ +131,5 @@
> +
> +template<typename T>
> +inline MoveRef<T> Move(const T& t)
> +{
> +  return MoveRef<T>(const_cast<T&>(t));

A comment here would be nice.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 14 • 11 years ago

Comment on attachment 772464 [details] [diff] [review]
3 - switch to MOZ_ASSERT

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

::: js/public/Vector.h
@@ +358,5 @@
>          return mBegin + mLength;
>      }
>  
>      T &operator[](size_t i) {
> +        MOZ_ASSERT(!entered && i < mLength);

Two asserts would be nicer

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 15 • 11 years ago

Comment on attachment 772468 [details] [diff] [review]
6 - Move ReentrancyGuard into its own header

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

::: mfbt/ReentrancyGuard.h
@@ +24,5 @@
> +#ifdef DEBUG
> +    ReentrancyGuard(T& obj)
> +      : entered(obj.entered)
> +#else
> +    ReentrancyGuard(T &/*obj*/)

& to the left

	Jeff Walden [:Waldo] Assignee
	Comment 16 • 11 years ago

Yeah, the style of all these patches/code is all over the map.  I figured it made slightly more sense to just get it done, then I'd go back over it with a style-comb afterward.

	Terrence Cole [:terrence]
	Comment 17 • 11 years ago

Comment on attachment 772460 [details] [diff] [review]
1 - Extract out the AllocPolicy bits

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

r=me

	Terrence Cole [:terrence]
	Comment 18 • 11 years ago

Comment on attachment 772463 [details] [diff] [review]
2 - mozilla/Move.h

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

r=me With my and Ms2ger's nits.

::: mfbt/Move.h
@@ +47,5 @@
> + *    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 MoveRef<T>
> + * referring to the same value; that's 1). An MoveRef<T> is simply a reference

s/An/A/

	Terrence Cole [:terrence]
	Comment 19 • 11 years ago

Comment on attachment 772464 [details] [diff] [review]
3 - switch to MOZ_ASSERT

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

r=me

	Terrence Cole [:terrence]
	Comment 20 • 11 years ago

Comment on attachment 772466 [details] [diff] [review]
4 - Use MOZ_*_INLINE

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

r=me

	Terrence Cole [:terrence]
	Comment 21 • 11 years ago

Comment on attachment 772467 [details] [diff] [review]
5 - Use MOZ_STATIC_ASSERT

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

r=me

	Terrence Cole [:terrence]
	Comment 22 • 11 years ago

Comment on attachment 772468 [details] [diff] [review]
6 - Move ReentrancyGuard into its own header

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

r=me

::: js/public/Utility.h
@@ +11,4 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Compiler.h"
>  #include "mozilla/Move.h"
> +#include "mozilla/ReentrancyGuard.h"

Is this used so many places that we cannot just IWYU?

	Terrence Cole [:terrence]
	Comment 23 • 11 years ago

Comment on attachment 772469 [details] [diff] [review]
7 - Move bit-counting stuff into MathAlgorithms.h, out of macros

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

r=me

::: js/src/ion/AsmJS.cpp
@@ +4828,4 @@
>  struct ParallelGroupState
>  {
>      WorkerThreadState &state;
> +    js::Vector<AsmJSParallelTask> &tasks;

I'm guessing this crept in from another patch? Doesn't really matter.

@@ +4989,4 @@
>  
>      // Allocate scoped AsmJSParallelTask objects. Each contains a unique
>      // LifoAlloc that provides all necessary memory for compilation.
> +    js::Vector<AsmJSParallelTask, 0> tasks(m.cx());

Ditto.

::: mfbt/MathAlgorithms.h
@@ +148,5 @@
> +#  define MOZ_BITSCAN_WINDOWS
> +
> +  extern "C" {
> +    unsigned char _BitScanReverse(unsigned long* Index, unsigned long mask);
> +    unsigned char _BitScanForward(unsigned long* Index, unsigned long mask);

Swap the order of these declarations to match the ones below.

::: mfbt/tests/TestCountZeroes.cpp
@@ +27,5 @@
> +  MOZ_ASSERT(CountLeadingZeroes64(0xF000F0F010000000) == 0);
> +  MOZ_ASSERT(CountLeadingZeroes64(0x70F080F000000001) == 1);
> +  MOZ_ASSERT(CountLeadingZeroes64(0x30F0F0F000100000) == 2);
> +  MOZ_ASSERT(CountLeadingZeroes64(0x10F0F05000000100) == 3);
> +  MOZ_ASSERT(CountLeadingZeroes64(0xF000F0F010000000) == 0);

This case is a duplicate of the first in this block.

	Terrence Cole [:terrence]
	Comment 24 • 11 years ago

Comment on attachment 772471 [details] [diff] [review]
8 - Move Swap over

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

r=me

	Terrence Cole [:terrence]
	Comment 25 • 11 years ago

Comment on attachment 772472 [details] [diff] [review]
Eliminate Vector's js/Utility.h dependency

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

r=me

	Terrence Cole [:terrence]
	Comment 26 • 11 years ago

Comment on attachment 772473 [details] [diff] [review]
10 - Introduce a TemplateLib.h with the stuff from js/TemplateLib.h that's needed, pare down that header

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

r=me

::: js/public/TemplateLib.h
@@ -109,2 @@
>  /*
>   * Traits class for identifying types that are implicitly barriered.

I've got a patch with r+ that kills this that I'm going to try to land soon. Whoever lands second is going to get to delete this file.

::: js/src/ds/BitArray.h
@@ +12,1 @@
>  #include "jstypes.h" 

Kill extra whitespace while you're here.

::: mfbt/TemplateLib.h
@@ +23,5 @@
> +namespace mozilla {
> +
> +namespace tl {
> +
> +/* Compute min/max. */

Single line comments should be //.

@@ +80,5 @@
> +{
> +    // Assert the precondition.  On success this evaluates to 0.  Otherwise it
> +    // triggers divide-by-zero at compile time: a guaranteed compile error in
> +    // C++11, and usually one in C++98.  Add this value to |value| to assure
> +    // its computation.

Multi-line comment should be /* */.

	Terrence Cole [:terrence]
	Comment 27 • 11 years ago

Comment on attachment 772475 [details] [diff] [review]
11 - Move/copy js/Vector.h to mozilla/Vector.h, have a new js/Vector.h implemented using mozilla/Vector.h

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

r=me

::: js/public/Vector.h
@@ +19,5 @@
> +//             size_t MinInlineCapacity = 0,
> +//             class AllocPolicy = TempAllocPolicy>
> +//   using Vector = mozilla::Vector<T, MinInlineCapacity, AllocPolicy>;
> +//
> +// ...and get rid of all the CRTP madness in mozilla::Vector(Base).  But we

Single space between sentences.

::: js/src/frontend/TokenStream.cpp
@@ +155,5 @@
>          // like that.
>          lineStartOffsets_[lineIndex] = lineStartOffset;
> +
> +        uint32_t maxPtr = MAX_PTR;
> +        (void)lineStartOffsets_.append(maxPtr);

Eww. Saw the comment above and am appalled, but will avoid scope-creeping you. :-)

::: mfbt/Vector.h
@@ +103,1 @@
>          MOZ_ASSERT(!v.usingInlineStorage());

Two space indenting throughout, unless you wanted to do that as a follow-up.

@@ +182,2 @@
>  /*
> + * A CRTP base class for vector-like classes.  Unless you really really want

One space between sentences. Comma between the adjacent adverbs really and really, unless you really meant for the first really to be an adjective.

@@ +191,2 @@
>  {
>      // typedef typename tl::StaticAssert<!tl::IsPostBarrieredType<T>::result>::result _;

Delete this.

@@ +191,4 @@
>  {
>      // typedef typename tl::StaticAssert<!tl::IsPostBarrieredType<T>::result>::result _;
>  
>      /* utilities */

// for single line comments throughout.

@@ +457,5 @@
>      T popCopy();
>  
>      /*
> +     * Transfers ownership of the internal buffer used by this vector to the
> +     * caller.  After this call, the vector is empty.  Since the returned

One space between sentences.

@@ +459,5 @@
>      /*
> +     * Transfers ownership of the internal buffer used by this vector to the
> +     * caller.  After this call, the vector is empty.  Since the returned
> +     * buffer may need to be allocated (if the elements are currently stored
> +     * in-place), the call can fail, returning NULL.

s/NULL/nullptr/

@@ +650,4 @@
>          /* This case occurs in ~15--20% of the calls to this function. */
>  
>          /*
> +         * Will mLength*4*sizeof(T) overflow?  This condition limits a vector

One space between sentences.

@@ +1091,5 @@
>  #endif
>  }
>  
> +/*
> + * STL-like container providing a short-lived, dynamic buffer.  Vector calls the

One space between sentences.

	Jeff Walden [:Waldo] Assignee
	Comment 28 • 11 years ago

The final bits of change, and the bit-counting portion, are still held up in try-servering or bug 891739.  But most of the rest is good to go.  Pushed, with more to come soon:

https://hg.mozilla.org/integration/mozilla-inbound/rev/272dce99bdd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8e8ebef928
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a88e4fd3a39
https://hg.mozilla.org/integration/mozilla-inbound/rev/6862050664f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9432b5f36fb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77abc3f3890
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f48a72285df

Minor responses to comments on these revs follow.

(In reply to :Ms2ger from comment #13)
> @@ +131,5 @@
> > +
> > +template<typename T>
> > +inline MoveRef<T> Move(const T& t)
> > +{
> > +  return MoveRef<T>(const_cast<T&>(t));
> 
> A comment here would be nice.

I don't understand quite what you're asking here.  But given this is how is is already, I figured it was reasonable to land this as-is and sort that out afterward.

(In reply to Terrence Cole [:terrence] from comment #22)
> ::: js/public/Utility.h
> @@ +11,4 @@
> >  #include "mozilla/Attributes.h"
> >  #include "mozilla/Compiler.h"
> >  #include "mozilla/Move.h"
> > +#include "mozilla/ReentrancyGuard.h"
> 
> Is this used so many places that we cannot just IWYU?

Nope, removed.

	Jeff Walden [:Waldo] Assignee
	Comment 29 • 11 years ago

...for clarification of the "comment would be nice" bit in comment 13, and what exactly is meant by it.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 30 • 11 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #29)
> ...for clarification of the "comment would be nice" bit in comment 13, and
> what exactly is meant by it.

Why you needed the const_cast.

	Ed Morley [:emorley]
	Comment 31 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/272dce99bdd9
https://hg.mozilla.org/mozilla-central/rev/2e8e8ebef928
https://hg.mozilla.org/mozilla-central/rev/8a88e4fd3a39
https://hg.mozilla.org/mozilla-central/rev/6862050664f3
https://hg.mozilla.org/mozilla-central/rev/9432b5f36fb8
https://hg.mozilla.org/mozilla-central/rev/c77abc3f3890
https://hg.mozilla.org/mozilla-central/rev/5f48a72285df

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 32 • 11 years ago

The obj.entered bit is unfortunate because it means you need to violate the Gecko style guide to use this there.

	Jeff Walden [:Waldo] Assignee
	Comment 34 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a4b367aae64 for the const_cast<> comment.  Still waiting on bug 891739 before I can land the rest of this.

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5a4b367aae64

	Jeff Walden [:Waldo] Assignee
	Comment 36 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/021fd4e03439
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab78d649fde6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8abf922fb3ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e972bc6d0f4

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/021fd4e03439
https://hg.mozilla.org/mozilla-central/rev/ab78d649fde6
https://hg.mozilla.org/mozilla-central/rev/8abf922fb3ea
https://hg.mozilla.org/mozilla-central/rev/7e972bc6d0f4