Closed
Bug 1287006
Opened 8 years ago
Closed 7 years ago
Optimize Maybe's members for packing
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
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.
Reporter | ||
Comment 1•8 years ago
|
||
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)
Reporter | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64424/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64424/
Assignee | ||
Comment 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
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-
Reporter | ||
Comment 9•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #5) > Jeff, were you planning on finishing this up? Yes.
Comment 10•8 years ago
|
||
(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 11•7 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Attachment #8771170 -
Attachment is obsolete: true
Comment 13•7 years ago
|
||
mozreview-review |
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 14•7 years ago
|
||
mozreview-review-reply |
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).
Comment 15•7 years ago
|
||
(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
Comment 16•7 years ago
|
||
It was Waldo... I probably should go sleeping...
Comment 17•7 years ago
|
||
Assuming alignas works like alignof, check out bug 1288016, and you'll see alignas might not help for better packing.
Comment 18•7 years ago
|
||
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.
Comment 19•7 years ago
|
||
(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.
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 21•7 years ago
|
||
This fixes Maybe. Let's call it a day in this bug and move on in other bugs.
Attachment #8831000 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Assignee: jgilbert → jwalden+bmo
Status: NEW → ASSIGNED
Comment 22•7 years ago
|
||
(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?
Assignee | ||
Comment 23•7 years ago
|
||
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.
Comment 24•7 years ago
|
||
(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)
Updated•7 years ago
|
Flags: needinfo?(jgilbert)
Comment 25•7 years ago
|
||
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+
Assignee | ||
Comment 26•7 years ago
|
||
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)
Assignee | ||
Comment 27•7 years ago
|
||
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)
Assignee | ||
Comment 28•7 years ago
|
||
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)
Assignee | ||
Comment 29•7 years ago
|
||
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)
Assignee | ||
Comment 30•7 years ago
|
||
Attachment #8836627 -
Flags: review?(nfroyd)
Assignee | ||
Comment 31•7 years ago
|
||
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)
Assignee | ||
Comment 32•7 years ago
|
||
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)
Assignee | ||
Comment 33•7 years ago
|
||
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 34•7 years ago
|
||
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 35•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8836627 -
Flags: review?(nfroyd) → review+
Comment 36•7 years ago
|
||
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 37•7 years ago
|
||
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 38•7 years ago
|
||
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+
Comment 39•7 years ago
|
||
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 40•7 years ago
|
||
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+
Assignee | ||
Comment 41•7 years ago
|
||
(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.
Assignee | ||
Comment 42•7 years ago
|
||
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).
Comment 43•7 years ago
|
||
(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.
Comment 44•7 years ago
|
||
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
Assignee | ||
Comment 45•7 years ago
|
||
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 46•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29d6afb1f916 https://hg.mozilla.org/mozilla-central/rev/deb448009371 https://hg.mozilla.org/mozilla-central/rev/91df292ad3ce https://hg.mozilla.org/mozilla-central/rev/07fffe40a023
Comment 47•7 years ago
|
||
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 48•7 years ago
|
||
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-
Assignee | ||
Comment 49•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8836630 -
Attachment is obsolete: true
Assignee | ||
Comment 50•7 years ago
|
||
Attachment #8839779 -
Flags: review?(docfaraday)
Comment 51•7 years ago
|
||
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+
Assignee | ||
Comment 52•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8839779 -
Attachment is obsolete: true
Comment 53•7 years ago
|
||
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+
Comment 54•7 years ago
|
||
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
Comment 55•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 56•7 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8c006751c6 Followup bustage fix. r=bustage in a CLOSED TREE
Comment 57•7 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/57775a0cc6f4 Final bustage followup. r=bustage in a still-CLOSED TREE
Comment 58•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5ddf5a0b85ae https://hg.mozilla.org/mozilla-central/rev/c34603e73ded https://hg.mozilla.org/mozilla-central/rev/e0f2e64261cc https://hg.mozilla.org/mozilla-central/rev/bc8c006751c6 https://hg.mozilla.org/mozilla-central/rev/57775a0cc6f4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1428664
You need to log in
before you can comment on or make changes to this bug.
Description
•