Closed Bug 1287006 Opened 3 years ago Closed 3 years ago

Optimize Maybe's members for packing

Categories

(Core :: MFBT, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- affected
firefox54 --- fixed

People

(Reporter: jgilbert, Assigned: Waldo)

References

Details

Attachments

(8 files, 4 obsolete files)

58 bytes, text/x-review-board-request
Waldo
: review-
Details
5.31 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
53.59 KB, patch
luke
: review+
Details | Diff | Splinter Review
23.48 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
20.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.14 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
16.57 KB, patch
Details | Diff | Splinter Review
14.38 KB, patch
bwc
: review+
Details | Diff | Splinter Review
Maybe can be packed more efficiently if:
* The bool is second, since it only needs to be aligned to bytes.
* AlignedStorage didn't always use at least 8 bytes via its uint64_t mDummy union member.
Review commit: https://reviewboard.mozilla.org/r/64422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64422/
Attachment #8771169 - Flags: review?(jwalden+bmo)
Attachment #8771170 - Flags: review?(jwalden+bmo)
Comment on attachment 8771170 [details]
Bug 1287006 - Put Maybe::mIsSome after mStorage to allow for possible efficient packing. -

https://reviewboard.mozilla.org/r/64424/#review62082
Attachment #8771170 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8771169 [details]
Bug 1287006 - Use alignas(). -

https://reviewboard.mozilla.org/r/64422/#review62108

The template thing is gunky and incomplete, and we have alignas support now.  So let's just use that.

::: mfbt/Alignment.h:144
(Diff revision 1)
>  struct AlignedStorage
>  {
>    union U
>    {
>      char mBytes[Nbytes];
> -    uint64_t mDummy;
> +    typename AlignedUint<Nbytes>::Type mDummy;

And this can be, instead of a union,

  alignas(Nbytes) char mBytes\[Nbytes\];

(see next comment)

::: mfbt/Alignment.h:164
(Diff revision 1)
>  struct MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS AlignedStorage2
>  {
>    union U
>    {
>      char mBytes[sizeof(T)];
> -    uint64_t mDummy;
> +    typename AlignedUint<sizeof(T)>::Type mDummy;

We can use alignas now, so instead of the union we can have

  alignas(T) char mBytes[sizeof(T)];

and things should be good -- better than with the current hack, even.
Attachment #8771169 - Flags: review?(jwalden+bmo) → review-
Jeff, were you planning on finishing this up?
Flags: needinfo?(jgilbert)
Duplicate of this bug: 1073559
Duplicate of this bug: 1127639
Duplicate of this bug: 1301004
(In reply to Nathan Froyd [:froydnj] from comment #5)
> Jeff, were you planning on finishing this up?

Yes.
(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> (In reply to Nathan Froyd [:froydnj] from comment #5)
> > Jeff, were you planning on finishing this up?
> 
> Yes.

Excellent.  Is it possible to get some static_assert tests that Maybe<T> is the expected size for various T as well?
Comment on attachment 8771170 [details]
Bug 1287006 - Put Maybe::mIsSome after mStorage to allow for possible efficient packing. -

https://reviewboard.mozilla.org/r/64424/#review103236

I don't think anything could be changed by putting `mIsSome` after `mStorage`, because the size of a class is always a multiple of its alignment, and data members would never overlap each other.
Attachment #8771170 - Attachment is obsolete: true
Comment on attachment 8771169 [details]
Bug 1287006 - Use alignas(). -

https://reviewboard.mozilla.org/r/64422/#review103344

::: mfbt/Alignment.h:145
(Diff revision 2)
>  
>  template<typename T>
> -struct MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS AlignedStorage2
> +struct MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS AlignedStorage2 final
>  {
> -  union U
> -  {
> +private:
> +  AlignedStorage<sizeof(T)> mStorage;

I wonder whether we can use `alignas(T) char mStorage[sizeof(T)];` directly.

Although I don't think there is any such usage at this moment, it is possible that someone may want to use Maybe with int128 or even 256bit integer (e.g. for SIMD) in the future, so I'd rather not rely on guessing alignment from the size.
Comment on attachment 8771169 [details]
Bug 1287006 - Use alignas(). -

https://reviewboard.mozilla.org/r/64422/#review103344

> I wonder whether we can use `alignas(T) char mStorage[sizeof(T)];` directly.
> 
> Although I don't think there is any such usage at this moment, it is possible that someone may want to use Maybe with int128 or even 256bit integer (e.g. for SIMD) in the future, so I'd rather not rely on guessing alignment from the size.

Looks like Waldo suggested the same thing months ago :) (Not sure how I missed that comment before I posting this).
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> Looks like Waldo suggested the same thing months ago :) (Not sure how I
> missed that comment before I posting this).

Oops, not Waldo, it was froydnj :P
It was Waldo... I probably should go sleeping...
Assuming alignas works like alignof, check out bug 1288016, and you'll see alignas might not help for better packing.
alignas(T) is equivalent to alignas(alignof(T)), so if we think alignof is unreliable (or undesirable), we can do alignas(MOZ_ALIGNOF(T)) instead.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)
> alignas(T) is equivalent to alignas(alignof(T)), so if we think alignof is
> unreliable (or undesirable), we can do alignas(MOZ_ALIGNOF(T)) instead.

This is not true. You cannot use MOZ_ALIGNOF with an abstract class. However, it seems we do use AlignedStorage2 with abstract class.
Comment on attachment 8771169 [details]
Bug 1287006 - Use alignas(). -

https://reviewboard.mozilla.org/r/64422/#review104008

As to the fixing/etc. of the two AlignedStorage types, independent of the `memcpyFrom` function being bad, it kinda seems more or less right, maybe.

But it's fixing tools that are unfortunately super-easy to misuse -- either you can only store trivially copyable stuff inside them (which isn't always the case), or you have to be very careful to explicitly placement-new objects of the desired kind into place, possibly move-construct them into new locations when copying them around, and ultimately destroy objects in those places (whether or not moved out).  It doesn't seem like a good idea to paint this sinking ship with a brand new coat of paint.

For now, I'd prefer a spot-fix to `Maybe` that fixes only that one class.  I'll just post the trivial patch to do that, and you can move on with your life.  (I, on the other hand, am probably going to have to follow up with peoples to fix the `UbiNode` code and to remove `AlignedStorage{,2}`.  Sest la vee.

::: js/public/UbiNode.h:376
(Diff revision 2)
>      // Because StackFrame is just a vtable pointer and an instance pointer, we
>      // can memcpy everything around instead of making concrete classes define
>      // virtual constructors. See the comment above Node's copy constructor for
>      // more details; that comment applies here as well.
>      StackFrame(const StackFrame& rhs) {
> -        memcpy(storage.u.mBytes, rhs.storage.u.mBytes, sizeof(storage.u));
> +        storage.memcpyFrom(rhs.storage);

This `memcpyFrom` function is Bad.  And it's Bad for the same reason bug 1269319 was Bad.

The so-called memcpy exception that permits writing the bytes of an object out via memcpy, then interpreting them as the type of the object copied, only applies for a *trivially copyable class*, C++14 [class]p9.  That requires the class have no non-trivial copy constructor.  But per C++14 [class.copy]p12, a class X has a trivial copy constructor only if class X has no virtual functions.

But this `storage` holds `BaseStackFrame` which has numerous virtual functions, e.g. `virtual uint64_t identifier() const`.  And the `storage` below holds `Base` which has numerous virtual functions, e.g. `virtual Id identifier() const`.  The only way to properly copy these things around, is to use move-construction from old location to new, properly destroying the moved-from location after.

So this is all Bad.  And really, adding a `memcpyFrom` function is extremely likely to *encourage* this bad pattern.  So it's a Bad idea to add it.
Attachment #8771169 - Flags: review?(jwalden+bmo) → review-
This fixes Maybe.  Let's call it a day in this bug and move on in other bugs.
Attachment #8831000 - Flags: review?(nfroyd)
Assignee: jgilbert → jwalden+bmo
Status: NEW → ASSIGNED
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21)
> Created attachment 8831000 [details] [diff] [review]
> Use |alignas(T) unsigned char mStorage[sizeof(T)]| instead of AlignedStorage
> inside Maybe
> 
> This fixes Maybe.  Let's call it a day in this bug and move on in other bugs.

Maybe we should add some static_assert(sizeof(Maybe<T>) ==/<= ...) tests?
You mean

  static_assert(sizeof(Maybe<T>) <= 2 * sizeof(T), "...");

?  I can tack one in, but it's weak enough it doesn't really do much.  And you can't strengthen it to less-than for most platforms, or equality because of m68k apparently.  I can add it in, certainly, but beyond negligible belt-and-suspendersness (which isn't bad, certainly) I'm not sure what it really does.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23)
> You mean
> 
>   static_assert(sizeof(Maybe<T>) <= 2 * sizeof(T), "...");
> 
> ?  I can tack one in, but it's weak enough it doesn't really do much.  And
> you can't strengthen it to less-than for most platforms, or equality because
> of m68k apparently.  I can add it in, certainly, but beyond negligible
> belt-and-suspendersness (which isn't bad, certainly) I'm not sure what it
> really does.

I was mostly thinking that sizeof(Maybe<T>) is pretty bad for small T (12 or 16 bytes for smaller-than-pointer-size) with the current, in-tree implementation, and having these static_asserts would ensure that we don't go back to that state of affairs.

And if we couldn't do equality because of m68k (maybe others, I suppose), we could #ifdef.  But at least the weaker <= would provide some assurance that our implementation is not terrible.  WDYT?
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jgilbert)
Comment on attachment 8831000 [details] [diff] [review]
Use |alignas(T) unsigned char mStorage[sizeof(T)]| instead of AlignedStorage inside Maybe

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

I think that static_asserts are a good idea.
Attachment #8831000 - Flags: review?(nfroyd) → review+
This has undergone some small changes since the last time.  In particular, there's some indirection to avoid a GCC -Wstrict-aliasing issue, and I added the desired static_asserts (I think).

But most important of all, I made Maybe<T> MOZ_NON_PARAM so that Maybe<T> can't be a parameter type.  MSVC has a compiler warning (error, because of our -Werror) if you try to pass a struct containing an alignas() member by value.  This is because alignas (in excess of types' natural alignment?) apparently doesn't modify ABI -- and so you're at the mercy of the platform ABI whether parameters actually respect alignas.

This is kind of nuts, IMO.  Excessive alignas *should* affect ABI.  But it doesn't.  Given that, I see no way we can ensure alignment of Maybe parameters.  So the best we can do is prohibit Maybe being a parameter type.

One small benefit of this: there are a surprising number of places accidentally passing Maybe by value, where the enclosed T is actually very big.  By converting these places to take |const Maybe&| instead, much code will actually become a lot more efficient.

Maybe<T> is a parameter type a bunch of different places, so I had to do treewide fixing for this.  I've split that up into a few patches to spread review load (and because a few places depend on parameter-type copying, so can't mechanically be made |const Maybe&| without stuff breaking).
Attachment #8836614 - Flags: review?(nfroyd)
Luke: the gist of it is, if you have |struct { alignas(8) char foo; }|, you can't use that as a parameter type because it isn't guaranteed to be 8-byte-aligned.  That's an ABI issue, and not all ABIs guarantee it.  To eliminate problems that would arise if passing such a struct by value (including compiler errors), we're going to forbid passing Maybe by value.  A static analysis pass will enforce this, so not just MSVC will fail.

Of particular note here: aside from much rote conversion of |Maybe| to |const Maybe&| in signatures, modifying Maybe also effects to make structs that *embed* it MOZ_NON_PARAM.  And one of those is |class MemoryAccessDesc|.  Incredibly, we *do* pass that by value now -- even though the head of the class indicates it's something we very much should avoid copying:

class MemoryAccessDesc
{
    uint32_t offset_;
    uint32_t align_;
    Scalar::Type type_;
    unsigned numSimdElems_;
    jit::MemoryBarrierBits barrierBefore_;
    jit::MemoryBarrierBits barrierAfter_;
    mozilla::Maybe<wasm::TrapOffset> trapOffset_;
    ...

I had to change several places from taking |Maybe|, to taking |Maybe*| and then being careful about doing things that modify the |Maybe|.  It's generally all straightforward, but it's still non-trivial changes.  Please examine changes to how |MemoryAccessDesc| is passed somewhat carefully.
Attachment #8836624 - Flags: review?(luke)
bz: it happens that to have Maybe<T> aligned sufficient for its possibly-owned T, using |alignas|, you then can't use |Maybe<T>| as parameter type.  (In essence alignas doesn't percolate into ABI, so function parameters aren't laid out preserving such alignment rules.)  Previous patches have made it so static analysis enforces that Maybe can't be used as a parameter (i.e. passed by value).  This adjust relevant bits of layout/ code, including a signature change in a virtual function in nsIFrame.  It's all |Maybe| -> |const Maybe&|, so there shouldn't be any fundamental trickiness to it, as long as the passed |Maybe| isn't modified by some other code in the pendency of the |const Maybe&|.
Attachment #8836625 - Flags: review?(bzbarsky)
The Maybe-won't-be-aligned-as-parameter restriction also applies to classes that include a Maybe member -- including Nullable.  So, have to pass Nullable by const& now.  No trickiness to these changes, just rote.
Attachment #8836626 - Flags: review?(bzbarsky)
We mostly discussed the MediaDecoderStateMachine.cpp changes already, but given the tricksiness of lifetime stuff and the need to copy even when passing by rvalue-reference, that bit wants more than the IRC once-over.

The WrapRunnableNM changes are also pretty consequential.  Two important things there.

First, you do not actually get perfect forwarding of constructor arguments if you don't have template constructor arguments.  Even if the args are class template parameters.  So instead of

template<typename... T>
struct S
{
  S(T&&... ts) : member(ts...) {}
};

in order to have perfect forwarding, you must do

template<typename... T>
struct S
{
  template<typename... Args>
  S(Args&&... args) : member(ts...) {}
};

even if ordinarily T and Args will be basically the same.

Second, the *stored* types for arguments should strip off references, etc.  This is what mozilla::Decay does -- it creates a storage type, that can be initialized by a reference if desired.  But you should *fill* those storage locations using the original type.  I'm not certain what the best way to explain this exact mechanism is, except to say it's the same sort of thing we use internally in Tuple/MakeTuple.  If desired, we could forward this bit of review to froydnj, too (or he could just driveby-look if he wants).

Conveniently, getting the Decay thing correct means we can simplify two WrapRunnableNM callers that were doing shenanigans because they didn't know how to pass a copy/addref'd version of a pointer.

Switching from std::sort to std::qsort in that one spot is unideal, but unavoidable.  We can't get STLs to not pass the sorted T by parameter (they could, but it's not a change we can force).  We could either write our own sort algorithm that did it -- probably not worth the trouble now.  Or we can sort with std::qsort in a type-unaware way, as if by memmoving.  Because JsepTrackPair doesn't contain anything that contains internal pointers, or requires out-of-band notification, I think we can safely do type-unaware copying.
Attachment #8836630 - Flags: review?(jwwang)
And, that's all the changes.  I'll squash them for the final landing, but they're largely split-upable for reviewing purposes.
Flags: needinfo?(jwalden+bmo)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a530b05b7b1f7ca6e4637485382b94ba51a0c49 indicates those patches should be landable (modulo any new Maybes/Nullables that have popped up since last rebase).  Importantly, there are no fundamental GCC/MSVC compiler issues that would require going back to a drawing board, tho.
Comment on attachment 8836624 [details] [diff] [review]
Adjust js/ code to not pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer

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

Makes sense.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +3375,5 @@
>      }
>  
>      // ptr and src must not be the same register.
>      // This may destroy ptr and src.
> +    MOZ_MUST_USE bool store(MemoryAccessDesc* access, RegI32 ptr, bool omitBoundsCheck, AnyReg src,

The load() function right above takes a mutable MemoryAccessDesc&; could you change it to a pointer to match?
Attachment #8836624 - Flags: review?(luke) → review+
Comment on attachment 8836614 [details] [diff] [review]
Use |alignas(T) unsigned char mStorage[sizeof(T)]| instead of AlignedStorage inside Maybe

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

r=me with slight changes below.  Thanks for adding the static_asserts.

::: mfbt/Maybe.h
@@ +454,5 @@
>    template<typename... Args>
>    void emplace(Args&&... aArgs)
>    {
>      MOZ_ASSERT(!mIsSome);
> +    ::new (data()) T(Forward<Args>(aArgs)...);

Can you file a followup to use our non-null-checking operator new for this?

::: mfbt/tests/TestMaybe.cpp
@@ +1078,5 @@
> +              "Maybe<int> shouldn't bloat");
> +static_assert(sizeof(Maybe<long>) <= 2 * sizeof(long),
> +              "Maybe<long> shouldn't bloat");
> +static_assert(sizeof(Maybe<double>) <= 2 * sizeof(double),
> +              "Maybe<long> shouldn't bloat");

Surely this message should talk about Maybe<double>?
Attachment #8836614 - Flags: review?(nfroyd) → review+
Attachment #8836627 - Flags: review?(nfroyd) → review+
Comment on attachment 8831000 [details] [diff] [review]
Use |alignas(T) unsigned char mStorage[sizeof(T)]| instead of AlignedStorage inside Maybe

I think this patch is obsolete now.
Attachment #8831000 - Attachment is obsolete: true
Comment on attachment 8836625 [details] [diff] [review]
Don't pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer, in layout/-related code

r=me
Attachment #8836625 - Flags: review?(bzbarsky) → review+
Comment on attachment 8836626 [details] [diff] [review]
Don't pass Nullable by value in various places, rather by const&

r=me
Attachment #8836626 - Flags: review?(bzbarsky) → review+
And please file a followup to delete dom::UnionMember's copy ctor and see whether that still works.  If not, we may have things to fix there.
Comment on attachment 8836630 [details] [diff] [review]
Adjust media/ code to not pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer

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

Hi Randell,
Can you review the webrtc code?

::: dom/media/webspeech/synth/SpeechSynthesisUtterance.cpp
@@ +157,5 @@
>  
>  void
>  SpeechSynthesisUtterance::DispatchSpeechSynthesisEvent(const nsAString& aEventType,
>                                                         uint32_t aCharIndex,
> +                                                       const Nullable<uint32_t>& aCharLength,

This one is tricky. Do we have to annotate Nullable like adding MOZ_NON_PARAM to SeeJob?

::: mfbt/IndexSequence.h
@@ -39,5 @@
>   *   Write a helper function which takes the tuple, and an index sequence
>   *   containing indices corresponding to the tuple indices.
>   *
>   *     template <typename... Args, size_t... Indices>
> - *     void Helper(const Tuple<Args...>& t, IndexSequence<Indices>)

Interesting. How could |IndexSequence<Indices>| not cause a compile error?
Attachment #8836630 - Flags: review?(rjesup)
Attachment #8836630 - Flags: review?(jwwang)
Attachment #8836630 - Flags: review+
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #40)
> > +                                                       const Nullable<uint32_t>& aCharLength,
> 
> This one is tricky. Do we have to annotate Nullable like adding
> MOZ_NON_PARAM to SeeJob?

It is so annotated, in a separate patch.  But froydnj argues for not annotating such things (because if the Maybe in Nullable were removed, it wouldn't be necessary), and it's a fair argument, with better compile error messages that shouldn't be hard to add.  So I'll likely be removing it before landing everything here.

> >   *     template <typename... Args, size_t... Indices>
> > - *     void Helper(const Tuple<Args...>& t, IndexSequence<Indices>)
> 
> Interesting. How could |IndexSequence<Indices>| not cause a compile error?

It did cause a compile error, when I copied the approach into the patchwork here.  :-)  Seemed like an obvious time to fix the docs, if I'd tried their approach and knew it to be buggy.
In principle we *could* just make static analysis detect any parameter that's a class, one of whose members has an alignas() attribute.  Then, no need for the attribute at all.  Given MOZ_NON_PARAM exists *only* for that purpose, it's something we could at least consider -- although EIBTI is at least one argument against doing so (not sure if it's persuasive or not, on first thought).
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #42)
> In principle we *could* just make static analysis detect any parameter
> that's a class, one of whose members has an alignas() attribute.  Then, no
> need for the attribute at all.  Given MOZ_NON_PARAM exists *only* for that
> purpose, it's something we could at least consider -- although EIBTI is at
> least one argument against doing so (not sure if it's persuasive or not, on
> first thought).

I filed bug 1339537 for this.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29d6afb1f916
Adjust js/ code to not pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer.  r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/deb448009371
Don't pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer, in layout/-related code.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/91df292ad3ce
Don't pass Nullable by value in various places, rather by const&.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/07fffe40a023
Don't pass Maybe by value in miscellaneous places.  r=froydnj
Landed a bunch of the signature-changing stuff that's independent of changes to Maybe -- haven't landed MOZ_NON_PARAM additions or the actual changes to Maybe yet.  Waiting on the last media review before those (minus the MOZ_NON_PARAM additions we don't want) can land.
Keywords: leave-open
Comment on attachment 8836630 [details] [diff] [review]
Adjust media/ code to not pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer

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

bwc for mtransport code
Attachment #8836630 - Flags: review?(rjesup)
Attachment #8836630 - Flags: review?(docfaraday)
Attachment #8836630 - Flags: review+
Comment on attachment 8836630 [details] [diff] [review]
Adjust media/ code to not pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer

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

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ +292,5 @@
>                                  SdpMediaSection::MediaType type) {
>      return GetTrack(mSessionAns, type, index);
>    }
>  
> +  static int ComparePairsByLevel(const void* lhs, const void* rhs) {

...this is gross. I would prefer to stop using Maybe in JsepTrackPair, and use -1 to indicate that it is not set.
Attachment #8836630 - Flags: review?(docfaraday) → review-
This is the same media patch, just with the r-'d unittest file changes excised -- posting for clarity, presuming prior r+s continue to hold.
Attachment #8836630 - Attachment is obsolete: true
Comment on attachment 8839779 [details] [diff] [review]
Make JsepTrackPair::mBundleLevel intptr_t instead of Maybe<size_t>, with -1 encoding the previous not-size_t state

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

r+ with nits

::: media/webrtc/signaling/src/jsep/JsepTrack.h
@@ +298,5 @@
>  // Need a better name for this.
>  struct JsepTrackPair {
>    size_t mLevel;
>    // Is this track pair sharing a transport with another?
> +  intptr_t mBundleLevel; // or -1 if no bundle level

Just make this an int, intptr_t might mislead the reader as to what this is used for.

@@ +307,5 @@
>    RefPtr<JsepTransport> mRtcpTransport;
> +
> +  bool HasBundleLevel() const {
> +    return mBundleLevel != -1;
> +  }

Throw in some vertical ws.

@@ +313,5 @@
> +    MOZ_ASSERT(HasBundleLevel());
> +    return size_t(mBundleLevel);
> +  }
> +  void SetBundleLevel(size_t aBundleLevel) {
> +    MOZ_ASSERT(0 <= aBundleLevel);

Isn't this guaranteed for the type?
Attachment #8839779 - Flags: review?(docfaraday) → review+
Couple reasons for the re-r?.

First, using |int| is super-unsatisfying to me when it could be smaller than |size_t| and I don't actually know/can't prove |int|'s smaller size is adequate to store all values.  So I didn't want to use |int| -- and in switching to just plain |size_t|, I realized the second problem.

Second, the previous patch neglected to initialize |mBundleLevel| in newly-created |JsepTrackPair| (which |Maybe<size_t>| would have auto-initialized without anyone doing anything).  I'm hoping this would have shown up on try, but I noticed it solely through inspection before reaching the try-testing state.  The fix uses C++11's new member-initializing syntax, which https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code notes is kosher to use.
Attachment #8840128 - Flags: review?(docfaraday)
Attachment #8839779 - Attachment is obsolete: true
Comment on attachment 8840128 [details] [diff] [review]
Make JsepTrackPair::mBundleLevel size_t instead of Maybe<size_t>, with SIZE_MAX encoding the previous not-size_t state

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

Ok.
Attachment #8840128 - Flags: review?(docfaraday) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddf5a0b85ae
Adjust media/ code to not pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer.  r=jw_wang, r=rjesup
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c34603e73ded
Make JsepTrackPair::mBundleLevel size_t instead of Maybe<size_t>, with SIZE_MAX encoding the previous not-size_t state.  (mBundleLevel counts things in memory, so SIZE_MAX is excluded from the typical semantics.)  r=bwc
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f2e64261cc
Use |alignas(T) unsigned char mStorage[sizeof(T)]| instead of AlignedStorage2 inside Maybe.  r=froydnj
Keywords: leave-open
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8c006751c6
Followup bustage fix.  r=bustage in a CLOSED TREE
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57775a0cc6f4
Final bustage followup.  r=bustage in a still-CLOSED TREE
Depends on: 1356063
You need to log in before you can comment on or make changes to this bug.