Closed Bug 891177 Opened 6 years ago Closed 6 years ago

Move js::Vector into mfbt

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(12 files)

4.22 KB, patch
terrence
: review+
Details | Diff | Splinter Review
20.15 KB, patch
terrence
: review+
Details | Diff | Splinter Review
8.13 KB, patch
terrence
: review+
Details | Diff | Splinter Review
4.58 KB, patch
terrence
: review+
Details | Diff | Splinter Review
757 bytes, patch
terrence
: review+
Details | Diff | Splinter Review
6.22 KB, patch
terrence
: review+
Details | Diff | Splinter Review
32.82 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.00 KB, patch
terrence
: review+
Details | Diff | Splinter Review
789 bytes, patch
terrence
: review+
Details | Diff | Splinter Review
17.39 KB, patch
terrence
: review+
Details | Diff | Splinter Review
66.60 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.35 KB, patch
luke
: review+
Details | Diff | Splinter Review
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.
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.  :-)
Attachment #772460 - Flags: review?(terrence)
Attachment #772463 - Flags: review?(terrence)
Attachment #772464 - Flags: review?(terrence)
Attachment #772466 - Flags: review?(terrence)
Attachment #772467 - Flags: review?(terrence)
Attachment #772468 - Flags: review?(terrence)
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.)
Attachment #772469 - Flags: review?(terrence)
mozilla/Move.h is arguably not quite the right place, but given it uses mozilla::Move it seems not totally unreasonable.
Attachment #772471 - Flags: review?(terrence)
And now, a few patches killing extraneous js/ dependencies in various ways.
Attachment #772472 - Flags: review?(terrence)
Arguably IsRelocatableMumble should be in a GC header, but please don't scope-creep me bro, it's an easy followup.
Attachment #772473 - Flags: review?(terrence)
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!
Attachment #772475 - Flags: review?(terrence)
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 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 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 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
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 on attachment 772460 [details] [diff] [review]
1 - Extract out the AllocPolicy bits

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

r=me
Attachment #772460 - Flags: review?(terrence) → review+
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/
Attachment #772463 - Flags: review?(terrence) → review+
Comment on attachment 772464 [details] [diff] [review]
3 - switch to MOZ_ASSERT

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

r=me
Attachment #772464 - Flags: review?(terrence) → review+
Comment on attachment 772466 [details] [diff] [review]
4 - Use MOZ_*_INLINE

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

r=me
Attachment #772466 - Flags: review?(terrence) → review+
Comment on attachment 772467 [details] [diff] [review]
5 - Use MOZ_STATIC_ASSERT

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

r=me
Attachment #772467 - Flags: review?(terrence) → review+
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?
Attachment #772468 - Flags: review?(terrence) → review+
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.
Attachment #772469 - Flags: review?(terrence) → review+
Comment on attachment 772471 [details] [diff] [review]
8 - Move Swap over

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

r=me
Attachment #772471 - Flags: review?(terrence) → review+
Comment on attachment 772472 [details] [diff] [review]
Eliminate Vector's js/Utility.h dependency

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

r=me
Attachment #772472 - Flags: review?(terrence) → review+
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 /* */.
Attachment #772473 - Flags: review?(terrence) → review+
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.
Attachment #772475 - Flags: review?(terrence) → review+
Depends on: 891739
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.
Whiteboard: [leave open]
...for clarification of the "comment would be nice" bit in comment 13, and what exactly is meant by it.
Flags: needinfo?(Ms2ger)
(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.
Flags: needinfo?(Ms2ger)
The obj.entered bit is unfortunate because it means you need to violate the Gecko style guide to use this there.
Attachment #775199 - Flags: review?(luke) → review+
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.
Depends on: 895879
Depends on: 895792
You need to log in before you can comment on or make changes to this bug.