891177 - Move js::Vector into mfbt

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 1 - Extract out the AllocPolicy bits

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.  :-)

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 2 - mozilla/Move.h

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 3 - switch to MOZ_ASSERT

Comment 4

[Jeff Walden [:Waldo]]

Attachment: 4 - Use MOZ_*_INLINE

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 5 - Use MOZ_STATIC_ASSERT

Comment 6

[Jeff Walden [:Waldo]]

Attachment: 6 - Move ReentrancyGuard into its own header

Comment 7

[Jeff Walden [:Waldo]]

Attachment: 7 - Move bit-counting stuff into MathAlgorithms.h, out of macros

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.)

Comment 8

[Jeff Walden [:Waldo]]

Attachment: 8 - Move Swap over

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

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Eliminate Vector's js/Utility.h dependency

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

Comment 10

[Jeff Walden [:Waldo]]

Attachment: 10 - Introduce a TemplateLib.h with the stuff from js/TemplateLib.h that's needed, pare down that header

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

Comment 11

[Jeff Walden [:Waldo]]

Attachment: 11 - Move/copy js/Vector.h to mozilla/Vector.h, have a new js/Vector.h implemented using mozilla/Vector.h

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!

Comment 12

[Jeff Walden [:Waldo]]

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.  :-)

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 16

[Jeff Walden [:Waldo]]

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.

Comment 17

[Terrence Cole [:terrence]]

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

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

r=me

Comment 18

[Terrence Cole [:terrence]]

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/

Comment 19

[Terrence Cole [:terrence]]

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

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

r=me

Comment 20

[Terrence Cole [:terrence]]

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

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

r=me

Comment 21

[Terrence Cole [:terrence]]

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

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

r=me

Comment 22

[Terrence Cole [:terrence]]

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?

Comment 23

[Terrence Cole [:terrence]]

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.

Comment 24

[Terrence Cole [:terrence]]

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

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

r=me

Comment 25

[Terrence Cole [:terrence]]

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

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

r=me

Comment 26

[Terrence Cole [:terrence]]

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 /* */.

Comment 27

[Terrence Cole [:terrence]]

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.

Comment 28

[Jeff Walden [:Waldo]]

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.

Comment 29

[Jeff Walden [:Waldo]]

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

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 31

[Ed Morley [:emorley]]

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

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 33

[Jeff Walden [:Waldo]]

Attachment: Add an explanatory comment by the const_cast<> in the Move(const T&) overload

Comment 34

[Jeff Walden [:Waldo]]

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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Jeff Walden [:Waldo]]

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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