Closed
Bug 1383816
Opened 7 years ago
Closed 7 years ago
Use a Variant in the implementation of FocusTarget
Categories
(Core :: Panning and Zooming, enhancement, P3)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: botond, Assigned: jeff.hajewski, Mentored)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(5 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
FocusTarget [1] is a type that represents one of three kinds of focus targets. It currently uses a hand-rolled tagged union in its implementation. When this code was reviewed (in bug 1351783), it was suggested that it might be better for the implementation to use mozilla::Variant instead. Since FocusTarget is sent over IPC, that first required adding IPC support to Variant. Since that's now done in bug 1371846, we can now try using Variant in FocusTarget. [1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/layers/apz/src/FocusTarget.h#25
Assignee | ||
Comment 1•7 years ago
|
||
I've started reviewing this and am happy to take this one on! I will check back in when I have a plan of attack.
Reporter | ||
Comment 2•7 years ago
|
||
Cool, thanks! One place that's good to know about is the ParamTraits specialization for FocusTarget [1], which will need changes corresponding to the changes made to FocusTarget itself. [1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/layers/ipc/LayersMessageUtils.h#451
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: [gfx-noted]
Random thought - :aosmond had some performance bottlenecks that were perhaps pointing to variant being expensive. It's all rumours at this point :), but if this is something that can have performance impact, it may be worth measuring before and after...
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #3) > Random thought - :aosmond had some performance bottlenecks that were perhaps > pointing to variant being expensive. It's all rumours at this point :), but > if this is something that can have performance impact, it may be worth > measuring before and after... I discussed this with :aosmond. In his scenario, a Variant was being created for every pixel in an image, so the overhead of using it added up. By comparison, we only have one FocusTarget per layers transaction (and on the compositor side, one per layer tree), so any overhead should be negligible. So, Jeff, you should feel free to continue working on this :)
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #4) Thanks for following up on this Botond! I'll keep plugging away
Assignee | ||
Comment 6•7 years ago
|
||
Alright I have a plan of attack here but could use some input on the preferred approach to include Variant. It seems I can forward declare the Variant class in the FocusTarget header if we make `mData` a pointer, otherwise we will need to include the Variant header in the FocusTarget header. Maybe this isn't a big deal, but I generally prefer to have as few includes as possible in the header. On the other hand, the current implementation doesn't treat `mData` as a pointer, so this would modify the structure a little bit. Otherwise, it seems like a simple change. Remove `mType`, make `mData` a `Variant<uint64_t, ScrollTargets>`, and in the IPC read/write methods we remove the `mType` read/writes and simply call the read/write for `mData`.
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Jeff Hajewski from comment #6) I was doing a little more digging around and it seems we may need to use a dummy type in the Variant as well. It doesn't appear that Variant can be empty, so maybe be default we have mData set to some dummy nonetype for when, previously, `mType = FocusTargetType::eNone`
Comment 8•7 years ago
|
||
(In reply to Jeff Hajewski from comment #6) > Alright I have a plan of attack here but could use some input on the > preferred approach to include Variant. It seems I can forward declare the > Variant class in the FocusTarget header if we make `mData` a pointer, > otherwise we will need to include the Variant header in the FocusTarget > header. Maybe this isn't a big deal, but I generally prefer to have as few > includes as possible in the header. On the other hand, the current > implementation doesn't treat `mData` as a pointer, so this would modify the > structure a little bit. I would recommend against using a pointer for mData and allocating it on the heap. FocusTarget is created frequently and I'd prefer to not have a heap allocation for this. We could optimize it so we don't recreate FocusTarget's but reuse them to get around that, but I don't see any downsides to including the Variant header.
Reporter | ||
Comment 9•7 years ago
|
||
+1 for storing the variant by value. The headers for common utility data structures like RefPtr, Maybe, and Variant are regularly included by other headers, so it's not a big deal. (In reply to Jeff Hajewski from comment #7) > I was doing a little more digging around and it seems we may need to use a > dummy type in the Variant as well. It doesn't appear that Variant can be > empty, so maybe be default we have mData set to some dummy nonetype for > when, previously, `mType = FocusTargetType::eNone` Right. We can use an empty stucture for this.
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #8) > I would recommend against using a pointer for mData and allocating it on the > heap. FocusTarget is created frequently and I'd prefer to not have a heap > allocation for this. Excellent point regarding heap allocation - I hadn't thought about it from that angle.
Assignee | ||
Comment 11•7 years ago
|
||
Sorry for the delay! I think I'm pretty close, just have a quick question on handling the switch statement [1] in FocusState.cpp. My inclination is to keep the switch statement, but since we are using a Variant we could also use a matcher. The problem with the matcher is accessing some of the members of the FocusState class. Do you all agree with the decision to stick with the switch? [1] https://dxr.mozilla.org/mozilla-central/rev/f0abd25e1f4acced652d180c34b7c9eda638deb1/gfx/layers/apz/src/FocusState.cpp#77
Reporter | ||
Comment 12•7 years ago
|
||
(In reply to Jeff Hajewski from comment #11) > Sorry for the delay! I think I'm pretty close, just have a quick question on > handling the switch statement [1] in FocusState.cpp. My inclination is to > keep the switch statement, but since we are using a Variant we could also > use a matcher. The problem with the matcher is accessing some of the members > of the FocusState class. Do you all agree with the decision to stick with > the switch? I think I would prefer using a matcher. The matcher can store a reference to the FocusState as a field, initialized with |*this| at the time the matcher is constructed, and access the felds of FocusState through that reference. The matcher can be declared as a local class (meaning, at function scope), to keep it close to where it's used.
Assignee | ||
Comment 13•7 years ago
|
||
I am pretty much done but want to run one more thing by you guys. For the matcher I pass in a reference to FocusState (as :botond mentioned) and also a reference to target. My reasoning is that there is a check and some indirection at [1] to get the FocusTarget target. Rather than repeat this, I pass it in as a const ref. Other than that, everything builds but I want to clean up the commits a bit and then it should be ready for review. [1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/FocusState.cpp?q=focusstate.cpp&redirect_type=direct#72
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Jeff Hajewski from comment #13) > I am pretty much done but want to run one more thing by you guys. For the > matcher I pass in a reference to FocusState (as :botond mentioned) and also > a reference to target. My reasoning is that there is a check and some > indirection at [1] to get the FocusTarget target. Rather than repeat this, I > pass it in as a const ref. > > Other than that, everything builds but I want to clean up the commits a bit > and then it should be ready for review. > > [1] > https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/FocusState. > cpp?q=focusstate.cpp&redirect_type=direct#72 After giving this some more thought, I'm only using target to access mSequenceNumber so it probably makes more sense to just pass that in instead of target itself.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → jeff.hajewski
Reporter | ||
Comment 21•7 years ago
|
||
Thanks for the patches, Jeff! I don't think we need to ask Ryan to review them as well. (He's welcome to have a look and comment if he'd like, of course, but we don't need to insist on it with an r?). In general, unless a patch is particularly tricky, or touches two different parts of the codebase that different people are familiar with, one reviewer per patch is sufficient.
Reporter | ||
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8904025 [details] Bug 1383816 - Fixes several minor errors in Variant's IPC implementation. https://reviewboard.mozilla.org/r/175760/#review180768 ::: commit-message-18552:4 (Diff revision 1) > +Bug 1383816 - Fixes severl minor errors in Variant's IPC implementation. r?botond, rhunt > + > +* VariantWriter construction switched to use brace initialization to avoid > + so-called "most vexing parse" The reason this change was necessary was not that the () syntax resulted in a vexing parse. It's that VariantWriter doesn't have a constructor defined, so we are initializing it using "aggregate initialization" (where the initializers are just matched one-to-one with the fields). Constructor initialization can be done using either the () or {} syntaxes (with the () syntax sometimes running into a vexing parse), but aggregate initialization needs to be done with the {} syntax. (Please adjust the commit message accordingly.) ::: commit-message-18552:5 (Diff revision 1) > +Bug 1383816 - Fixes severl minor errors in Variant's IPC implementation. r?botond, rhunt > + > +* VariantWriter construction switched to use brace initialization to avoid > + so-called "most vexing parse" > +* Call to AsVariant was in appropriately called via |paramType| when it should nit: "inappropriately" (one word)
Reporter | ||
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8904025 [details] Bug 1383816 - Fixes several minor errors in Variant's IPC implementation. https://reviewboard.mozilla.org/r/175760/#review180774 ::: commit-message-18552:1 (Diff revision 1) > +Bug 1383816 - Fixes severl minor errors in Variant's IPC implementation. r?botond, rhunt nit: typo ("several")
Reporter | ||
Comment 24•7 years ago
|
||
(More reviews to come later today.)
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #21) > Thanks for the patches, Jeff! > > I don't think we need to ask Ryan to review them as well. (He's welcome to > have a look and comment if he'd like, of course, but we don't need to insist > on it with an r?). > > In general, unless a patch is particularly tricky, or touches two different > parts of the codebase that different people are familiar with, one reviewer > per patch is sufficient. Thanks for the info -- I wasn't sure about reviewers but since he was participating I figured I'd add him as well. I will limit it to one reviewer (when appropriate) in the future. In terms of structuring smaller commits, was my approach ok or could I have improved them? I was thinking about the push over breakfast and thought maybe the |mType| commit should have been merged into the commit on the same file, rather than split across two commits. Thanks for all the comments so far!
Reporter | ||
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8904025 [details] Bug 1383816 - Fixes several minor errors in Variant's IPC implementation. https://reviewboard.mozilla.org/r/175760/#review180798
Attachment #8904025 -
Flags: review?(botond)
Reporter | ||
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8904022 [details] Bug 1383816 - Adds Variant in implementation of FocusTarget and removes union; https://reviewboard.mozilla.org/r/175754/#review180794 ::: gfx/layers/apz/src/FocusTarget.h:57 (Diff revision 1) > > // Whether there are keydown, keypress, or keyup event listeners > // in the event target chain of the focused element > bool mFocusHasKeyEventListeners; > > - FocusTargetType mType; > + mozilla::Variant<uint64_t, ScrollTargets, NoFocusTarget> mData = AsVariant(NoFocusTarget()); One thing that's lost in this transformation, is the identifier "mRefLayerId" which served as a description for what the uint64_t variant signifies. To preserve this description, let's do: typedef uint64_t RefLayerId; and then: Variant<RefLayerId, ScrollTargets, NoFocusTarget> mData = ...; We can put RefLayerId up near where NoFocusTarget is defined. ::: gfx/layers/apz/src/FocusTarget.cpp:175 (Diff revision 1) > FT_LOG("Creating reflayer target with seq=%" PRIu64 ", kl=%d, lt=%" PRIu64 "\n", > aFocusSequenceNumber, > mFocusHasKeyEventListeners, > rfp->GetLayersId()); > > - mType = FocusTarget::eRefLayer; > + mData = AsVariant(rfp->GetLayersId()); For improved readability, I would suggest explicitly specifying the type here: mData = AsVariant<RefLayerId>(rfp->GetLayerId()); This retains the property of the previous code that we are explicitly specifying which of the three variants we are choosing (rather than letting that be implied by the type of the argument). ::: gfx/layers/apz/src/FocusTarget.cpp:212 (Diff revision 1) > presShell->GetScrollableFrameToScrollForContent(selectedContent.get(), > nsIPresShell::eVertical); > > // We might have the globally focused element for scrolling. Gather a ViewID for > // the horizontal and vertical scroll targets of this element. > - mType = FocusTarget::eScrollLayer; > + // Note: we can't directly access mData's data without first extracting it, so I would remove this comment, as it's slightly misleading. You *can* access the ScrollTargets stored inside the variant directly, if that is in fact what the variant is currently storing. But we don't know that. We could make sure that's the case, like so: mData = AsVariant(ScrollTargets{}); and then assign to it directly: mData.as<ScrollTargets>().mHorizontal = horizontal; mData.as<ScrollTargets>().mVertical = vertical; However, that's not necessarily an improvement over what you have, so keeping what you have is fine (just remove the comment). ::: gfx/layers/apz/src/FocusTarget.cpp:232 (Diff revision 1) > bool > FocusTarget::operator==(const FocusTarget& aRhs) const > { > if (mSequenceNumber != aRhs.mSequenceNumber || > mFocusHasKeyEventListeners != aRhs.mFocusHasKeyEventListeners || > - mType != aRhs.mType) { > + mData.as<uint64_t>() != aRhs.mData.as<uint64_t>()) { |mData.as<uint64_t>()| is not equivalent to |mType| in the previous version: |mType| was the discriminator, |mData.as<uint64_t>()| is one of the variant values. Moreover, |mData.as<uint64_t>| will assert if the variant isn't currently storing that type. The equivalent to |mType| would be |mData.tag|, except that's a private field. Thankfully, Variant provides an operator== that handles the comparison for us, so we can simplify this whole function to: return mSequenceNumber == aRhs.mSequenceNumber && mFocusHasKeyEventListeners == aRhs.mFocusHasKeyEventListeners && mData == aRhs.mData;
Attachment #8904022 -
Flags: review?(botond)
Reporter | ||
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8904024 [details] Bug 1383816 - Adds IPC Read and Write methods for FocusTarget and NoFocusState structs and creates EmptyStructSerializer helper class; https://reviewboard.mozilla.org/r/175758/#review180800 ::: commit-message-de70c:1 (Diff revision 1) > +Adds IPC Read and Write methods for Variant and NoFocusState structs; r?botond, rhunt This patch is not adding Read and Write methods for Variant. ::: gfx/layers/ipc/LayersMessageUtils.h:451 (Diff revision 1) > mozilla::layers::FocusTarget::eNone, > mozilla::layers::FocusTarget::sHighestFocusTargetType> > {}; > > template <> > +struct ParamTraits<mozilla::layers::FocusTarget::NoFocusTarget> For bonus points, define an "EmptyStructSerializer" class, and have ParamTraits<NoFocusTarget> inherit from EmptyStructSerializer. This way, someone else needing to serialize an empty struct in the future can avoid repeating the definitions of the no-op Read and Write methods. (See PlainOldDataSerializer [1] and an example use of it [2], for a similar pattern.) [1] http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/ipc/glue/IPCMessageUtils.h#274 [2] http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/gfx/layers/ipc/LayersMessageUtils.h#632
Attachment #8904024 -
Flags: review?(botond)
Reporter | ||
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8904027 [details] Bug 1383816 - Deletes extraneous mType statement from FocusTarget.cpp; https://reviewboard.mozilla.org/r/175764/#review180802 Please use histedit to fold this patch into the first patch.
Attachment #8904027 -
Flags: review?(botond)
Reporter | ||
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8904026 [details] Bug 1383816 - Removes FocusTargetType ParamTraits specialization; https://reviewboard.mozilla.org/r/175762/#review180804
Attachment #8904026 -
Flags: review?(botond) → review+
Reporter | ||
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8904023 [details] Bug 1383816 - Updates FocusState to use variant matcher; https://reviewboard.mozilla.org/r/175756/#review180806 ::: commit-message-d0718:8 (Diff revision 1) > +Updates FocusState::Update to use a matcher on the the mData variant. This > +replaces a switch statement that was used to unpack the mData union, prior to > +making mData a variant. > + > +FocusTargetDataMatcher has three match methods, which are largely unmodified > +from the three cases of theoriginal switch statement. The constructor takes two nit: space ("the original") ::: gfx/layers/apz/src/FocusState.cpp:80 (Diff revision 1) > mFocusHasKeyEventListeners |= target.mFocusHasKeyEventListeners; > > - switch (target.mType) { > - case FocusTarget::eRefLayer: { > + // Match on the data stored in mData > + struct FocusTargetDataMatcher { > + > + FocusTargetDataMatcher(FocusState* focusState, const uint64_t sequenceNumber) : This constructor can be omitted; we can use aggregate initialization to initialize the structure instead (will require using the {} syntax as mentioned). ::: gfx/layers/apz/src/FocusState.cpp:83 (Diff revision 1) > - case FocusTarget::eRefLayer: { > + struct FocusTargetDataMatcher { > + > + FocusTargetDataMatcher(FocusState* focusState, const uint64_t sequenceNumber) : > + focusState(focusState), sequenceNumber(sequenceNumber) {} > + > + FocusState* focusState; I would prefer using a reference rather than a pointer. Also, please use the |mFoo| naming convention for data members. ::: gfx/layers/apz/src/FocusState.cpp:86 (Diff revision 1) > + focusState(focusState), sequenceNumber(sequenceNumber) {} > + > + FocusState* focusState; > + const uint64_t sequenceNumber; > + > + void match(const FocusTarget::NoFocusTarget& noFocusTarget) { and |aFoo| for parameters. (Why |aFoo| and not |pFoo|, you ask? Whoever came up with the convention wasn't clear on the distinction between "parameters" and "arguments" :p). ::: gfx/layers/apz/src/FocusState.cpp:93 (Diff revision 1) > + > + // Mark what sequence number this target has for debugging purposes so > + // we can always accurately report on whether we are stale or not > + focusState->mLastContentProcessedEvent = sequenceNumber; > + > + return; There's no need to have a return statement at the end of a void function. ::: gfx/layers/apz/src/FocusState.cpp:110 (Diff revision 1) > - FS_LOG("Looking for target in lt=%" PRIu64 "\n", target.mData.mRefLayerId); > + FS_LOG("Looking for target in lt=%" PRIu64 "\n", refLayerId); > > // The focus target is in a child layer tree > - mFocusLayersId = target.mData.mRefLayerId; > - break; > + focusState->mFocusLayersId = refLayerId; > + > + return; Likewise ::: gfx/layers/apz/src/FocusState.cpp:134 (Diff revision 1) > // numbers. > - if (mLastAPZProcessedEvent == 1 && > - mLastContentProcessedEvent > mLastAPZProcessedEvent) { > - mLastAPZProcessedEvent = mLastContentProcessedEvent; > + if (focusState->mLastAPZProcessedEvent == 1 && > + focusState->mLastContentProcessedEvent > focusState->mLastAPZProcessedEvent) { > + focusState->mLastAPZProcessedEvent = focusState->mLastContentProcessedEvent; > } > return; Likewise ::: gfx/layers/apz/src/FocusState.cpp:136 (Diff revision 1) > - mLastContentProcessedEvent > mLastAPZProcessedEvent) { > - mLastAPZProcessedEvent = mLastContentProcessedEvent; > + focusState->mLastContentProcessedEvent > focusState->mLastAPZProcessedEvent) { > + focusState->mLastAPZProcessedEvent = focusState->mLastContentProcessedEvent; > } > return; > } > - case FocusTarget::eNone: { > + }; // struct FocusTargetMatcher FocusTargetDataMatcher
Attachment #8904023 -
Flags: review?(botond)
Reporter | ||
Comment 32•7 years ago
|
||
Thanks, Jeff, these patches generally look really good! I have a few comments which I posted. The workflow I recommend for addressing them is to do a "histedit" on the patch series, and use "edit" on the patches that need to be changed. When doing the "histedit", I'd like to ask you to do the following as well: - Use "fold" (or "roll") to combine the "Deletes extraneous mType statement from FocusTarget.cpp" patch into the "Adds Variant in implementation of FocusTarget and removes union" patch (as mentioned). - Re-order the patches so that "Fixes several minor errors in Variant's IPC implementation" is the first one. It makes sense for that to go first since it's a general fix to Variant, while the remaining patches are specific to FocusTarget.
Updated•7 years ago
|
Attachment #8904022 -
Flags: review?(rhunt)
Updated•7 years ago
|
Attachment #8904023 -
Flags: review?(rhunt)
Updated•7 years ago
|
Attachment #8904024 -
Flags: review?(rhunt)
Updated•7 years ago
|
Attachment #8904025 -
Flags: review?(rhunt)
Updated•7 years ago
|
Attachment #8904026 -
Flags: review?(rhunt)
Updated•7 years ago
|
Attachment #8904027 -
Flags: review?(rhunt)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904027 -
Attachment is obsolete: true
Assignee | ||
Comment 38•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #27) > Thankfully, Variant provides an operator== that handles the comparison for > us, so we can simplify this whole function to: > > return mSequenceNumber == aRhs.mSequenceNumber && > mFocusHasKeyEventListeners == aRhs.mFocusHasKeyEventListeners && > mData == aRhs.mData; Since Variant relies on the operator== from the types used in an instance of variant I had to add operator== for both ScrollTargets and NoFocusTarget. I just pushed the patches for review. I wasn't sure how to somehow exclude the patch you already approved, so it is included with the group. I did modify its commit message to remove Ryan from the reviewer request.
Reporter | ||
Comment 39•7 years ago
|
||
(In reply to Jeff Hajewski from comment #38) > (In reply to Botond Ballo [:botond] from comment #27) > > Thankfully, Variant provides an operator== that handles the comparison for > > us, so we can simplify this whole function to: > > > > return mSequenceNumber == aRhs.mSequenceNumber && > > mFocusHasKeyEventListeners == aRhs.mFocusHasKeyEventListeners && > > mData == aRhs.mData; > > Since Variant relies on the operator== from the types used in an instance of > variant I had to add operator== for both ScrollTargets and NoFocusTarget. Good catch, thanks! > I just pushed the patches for review. I wasn't sure how to somehow exclude > the patch you already approved, so it is included with the group. I did > modify its commit message to remove Ryan from the reviewer request. Yep, that's fine, MozReview handles this. As you can see, it preserved the r+ for the patch I already reviewed.
Reporter | ||
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8904022 [details] Bug 1383816 - Adds Variant in implementation of FocusTarget and removes union; https://reviewboard.mozilla.org/r/175754/#review181892 ::: commit-message-37824:19 (Diff revision 2) > +* Updates methods using mData to use proper variant methods > +* Removes references to mType, instead using the appropriate > + variant methods > + > +MozReview-Commit-ID: BAarVxSGDtJ > +*** These extra lines in the commit message, which originate from the commit message of the patch that was folded in, can be removed. (You can use the "mess" command in histedit to edit a commit message.)
Attachment #8904022 -
Flags: review?(botond)
Reporter | ||
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8904025 [details] Bug 1383816 - Fixes several minor errors in Variant's IPC implementation. https://reviewboard.mozilla.org/r/175760/#review181896
Attachment #8904025 -
Flags: review?(botond) → review+
Reporter | ||
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8904023 [details] Bug 1383816 - Updates FocusState to use variant matcher; https://reviewboard.mozilla.org/r/175756/#review181898
Attachment #8904023 -
Flags: review?(botond) → review+
Reporter | ||
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8904024 [details] Bug 1383816 - Adds IPC Read and Write methods for FocusTarget and NoFocusState structs and creates EmptyStructSerializer helper class; https://reviewboard.mozilla.org/r/175758/#review181900
Attachment #8904024 -
Flags: review?(botond) → review+
Reporter | ||
Comment 44•7 years ago
|
||
Thanks Jeff! As the only remaining comment concerns a commit message, I went ahead and pushed the patches to Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=843cbe2f1c81b261fe2f838a3f1b2ef4b8f06927
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 50•7 years ago
|
||
mozreview-review |
Comment on attachment 8904022 [details] Bug 1383816 - Adds Variant in implementation of FocusTarget and removes union; https://reviewboard.mozilla.org/r/175754/#review181926
Attachment #8904022 -
Flags: review?(botond) → review+
Reporter | ||
Comment 51•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #44) > As the only remaining comment concerns a commit message, I went ahead and > pushed the patches to Try: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=843cbe2f1c81b261fe2f838a3f1b2ef4b8f06927 Hmm. Try does not seem happy - all the tests are failing. It's not clear to me why...
Assignee | ||
Comment 52•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #51) > (In reply to Botond Ballo [:botond] from comment #44) > > As the only remaining comment concerns a commit message, I went ahead and > > pushed the patches to Try: > > > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=843cbe2f1c81b261fe2f838a3f1b2ef4b8f06927 > > Hmm. Try does not seem happy - all the tests are failing. It's not clear to > me why... I will try running them locally
Reporter | ||
Comment 53•7 years ago
|
||
I triggered a Try push for the baseline changeset of this patch series, and that looks pretty good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc11791891614292ee775e7a17f6f0cad868f79f So, the failures do appear to somehow be related to these patches, as opposed to some general issue in our automation infrastructure that's affecting all tests.
Reporter | ||
Comment 54•7 years ago
|
||
One thing I notice is that non-e10s test suites (e.g. "tc-M") are passing, while e10s test suites (e.g. "tc-M-e10s") are failing. We do a lot more IPC in e10s mode (because there are two processes that need to communicate), so this suggests the problem is related to the IPC.
Assignee | ||
Comment 55•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #54) > One thing I notice is that non-e10s test suites (e.g. "tc-M") are passing, > while e10s test suites (e.g. "tc-M-e10s") are failing. We do a lot more IPC > in e10s mode (because there are two processes that need to communicate), so > this suggests the problem is related to the IPC. Do you have any suggestions on how I should go about debugging this other than just wading through the logs? I ran the tests locally (xpcshell), and got about 400 failures and about 7,200 successes. Some of the failures mention IPC, but others are hard to tell (e.g., a bookmarks test which may or may not have failed because of IPC issues).
Reporter | ||
Comment 56•7 years ago
|
||
(In reply to Jeff Hajewski from comment #55) > Do you have any suggestions on how I should go about debugging this other > than just wading through the logs? > > I ran the tests locally (xpcshell), and got about 400 failures and about > 7,200 successes. Some of the failures mention IPC, but others are hard to > tell (e.g., a bookmarks test which may or may not have failed because of IPC > issues). Here's one approach you could take: - Pick one specific test which failed in the Try run. - Without your patches applied, add logging to ParamTraits<FocusTarget>::Read() and Write() (for Read(), log whether the read was successful). - Run the test without your patches applied but with the logging, and see how many times FocusTarget is written and read during that test run. - Apply your patches and the logging, and see how many times FocusTarget is written and read now, and whether the reads are still successful. If there is a difference in the logs, and in particular if reads are failing with your patches applied, then you have something you can debug.
Reporter | ||
Comment 57•7 years ago
|
||
(I did look over the patches and bug 1371846 a second time to see if I could spot anything that would cause an IPC failure, but nothing jumped out at me.)
Assignee | ||
Comment 58•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #56) > (In reply to Jeff Hajewski from comment #55) > > Do you have any suggestions on how I should go about debugging this other > > than just wading through the logs? > > > > I ran the tests locally (xpcshell), and got about 400 failures and about > > 7,200 successes. Some of the failures mention IPC, but others are hard to > > tell (e.g., a bookmarks test which may or may not have failed because of IPC > > issues). > > Here's one approach you could take: > > - Pick one specific test which failed in the Try run. > > - Without your patches applied, add logging to > ParamTraits<FocusTarget>::Read() and Write() (for > Read(), log whether the read was successful). > > - Run the test without your patches applied but with > the logging, and see how many times FocusTarget > is written and read during that test run. > > - Apply your patches and the logging, and see how > many times FocusTarget is written and read now, and > whether the reads are still successful. > > If there is a difference in the logs, and in particular if reads are failing > with your patches applied, then you have something you can debug. I found this[1] info on logging. Is this what I should be using or is there some other logging I should use? I haven't dug too deep into the testing docs, but is there a way to rerun failed tests rather than the whole test suite? While the tests didn't take forever, they also weren't particularly fast. I guess I'll just need to make sure I get it right the first time :) [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Gecko_Logging
Reporter | ||
Comment 59•7 years ago
|
||
(In reply to Jeff Hajewski from comment #58) > I found this[1] info on logging. Is this what I should be using or is there > some other logging I should use? I generally use printf_stderr. It behaves just like printf, but goes to standard error instead of standard output. What platform are you run? On Windows, looking at the standard error can be a bit tricky, but on Mac or Linux it "just works" (i.e. shows up in the terminal). > I haven't dug too deep into the testing docs, but is there a way to rerun > failed tests rather than the whole test suite? While the tests didn't take > forever, they also weren't particularly fast. I guess I'll just need to make > sure I get it right the first time :) I'm not sure, to be honest. However, note that my suggestion from comment 56 involves running just a single test. That shouldn't take too long.
Assignee | ||
Comment 60•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #59) > I generally use printf_stderr. It behaves just like printf, but goes to > standard error instead of standard output. > > What platform are you run? On Windows, looking at the standard error can be > a bit tricky, but on Mac or Linux it "just works" (i.e. shows up in the > terminal). I'll be running them on Linux so I will use printf_stderr > I'm not sure, to be honest. However, note that my suggestion from comment 56 > involves running just a single test. That shouldn't take too long. Ah yes... good point :)
Comment 61•7 years ago
|
||
mozreview-review |
Comment on attachment 8904023 [details] Bug 1383816 - Updates FocusState to use variant matcher; https://reviewboard.mozilla.org/r/175756/#review182152 ::: gfx/layers/apz/src/FocusState.cpp (Diff revision 3) > // numbers. > - if (mLastAPZProcessedEvent == 1 && > - mLastContentProcessedEvent > mLastAPZProcessedEvent) { > - mLastAPZProcessedEvent = mLastContentProcessedEvent; > + if (mFocusState.mLastAPZProcessedEvent == 1 && > + mFocusState.mLastContentProcessedEvent > mFocusState.mLastAPZProcessedEvent) { > + mFocusState.mLastAPZProcessedEvent = mFocusState.mLastContentProcessedEvent; > - } > - return; This return statement (and the others in the switch) are not preserved in the conversion to use a matcher. This will result in infinite loops in the compositor which is probably what you're seeing on try. I think you can convert ::match to return a boolean and use that to determine when we need to bail out of the loop.
Comment 62•7 years ago
|
||
mozreview-review |
Comment on attachment 8904022 [details] Bug 1383816 - Adds Variant in implementation of FocusTarget and removes union; https://reviewboard.mozilla.org/r/175754/#review182154 ::: gfx/layers/apz/src/FocusTarget.h:49 (Diff revision 3) > + // because we can't have an empty variant > + struct NoFocusTarget { > + bool operator==(const NoFocusTarget& aRhs) const > - { > + { > - uint64_t mRefLayerId; > - ScrollTargets mScrollTargets; > + return true; > + } nit: trailing whitespace ::: gfx/layers/apz/src/FocusTarget.h:70 (Diff revision 3) > > // Whether there are keydown, keypress, or keyup event listeners > // in the event target chain of the focused element > bool mFocusHasKeyEventListeners; > > - FocusTargetType mType; > + mozilla::Variant<RefLayerId, ScrollTargets, NoFocusTarget> mData = AsVariant(NoFocusTarget()); nit: I'm not sure if there's a compiler reason for this, but I'd prefer to initialize this in the constructors of FocusTarget. It's then in the same place as the other member's initializations.
Comment 63•7 years ago
|
||
mozreview-review |
Comment on attachment 8904023 [details] Bug 1383816 - Updates FocusState to use variant matcher; https://reviewboard.mozilla.org/r/175756/#review182158 ::: gfx/layers/apz/src/FocusState.cpp:129 (Diff revision 3) > - mLastContentProcessedEvent = target.mSequenceNumber; > - return; > - } > + } > - } > + } > + }; // struct FocusTargetDataMatcher > + nit: trailing whitespace
Comment 64•7 years ago
|
||
Sorry about the drive by reviews, I saw there were some interesting test failures and wanted to take a look. :)
Reporter | ||
Comment 65•7 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #61) > This return statement (and the others in the switch) are not preserved in > the conversion to use a matcher. This will result in infinite loops in the > compositor which is probably what you're seeing on try. Oh, nice catch! I totally overlooked that.
Assignee | ||
Comment 66•7 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #64) > Sorry about the drive by reviews, I saw there were some interesting test > failures and wanted to take a look. :) Thanks for all the input Ryan! I've fixed the nits and am looking at the matcher now.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 72•7 years ago
|
||
Alright I made some updates based on Ryan's feedback. I tried pushing to Try but was not successful (error message said something about remote having heads not known locally and that my push would create an additional head...) I guess I'll leave this to the pros for now. After looking at the switch statement, it appears the only case we don't break out of the loop is when |mFocusLayersId != target.mData.mRefLayerId| for the RefLayerId case. I decided to have the matcher return |true| if we should return after the matcher finishes, and |false| otherwise. As you can see in the diff, the matcher returns |false| in the place where the switch had |break|. I'm not convinced this fixes the issue, so either there is a bug with the fix or there is something additional going on here. Although I couldn't push to Try, I ran tests locally and got the same number of failures. Just as a heads up, I will be away most of today (I'll have about an hour early in the evening) and the earliest I'll be able to make any major updates will be tomorrow morning.
Reporter | ||
Comment 73•7 years ago
|
||
(In reply to Jeff Hajewski from comment #72) > I tried pushing to Try > but was not successful (error message said something about remote having > heads not known locally and that my push would create an additional head...) > I guess I'll leave this to the pros for now. How were you trying to push? Do you have Level 1 commit access [1]? In any case, I did a new Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7713d5469318 [1] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
Assignee | ||
Comment 74•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #73) > How were you trying to push? hg push -r . try > Do you have Level 1 commit access [1]? Yes - I was granted this a while back (sometime in July maybe), but haven't pushed to Try yet. > In any case, I did a new Try push: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7713d5469318 Excellent! I'll keep an eye on this and see what happens but I'm not optimistic
Reporter | ||
Comment 75•7 years ago
|
||
(In reply to Jeff Hajewski from comment #74) > (In reply to Botond Ballo [:botond] from comment #73) > > How were you trying to push? > hg push -r . try Do you have "try" set up as a path in <srcdir>/.hg/hgrc? Or do you have the "firefoxtree" extension enabled (typically it would be in ~/.hgrc, under [extensions])?
Assignee | ||
Comment 76•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #75) > Do you have "try" set up as a path in <srcdir>/.hg/hgrc? Or do you have the > "firefoxtree" extension enabled (typically it would be in ~/.hgrc, under > [extensions])? I'll have to check when I get home. It was a while ago when I set it up but I used the setup wizard so whatever the default/preferred approach is probably what I have. This push to try is looking promising. Two failures that appear to be intermittent failures (bug 1357082 and bug 1394982) but still a couple of hours until the last test runs. If this is successful, I'll need to spend some time figuring out why my local tests are failing...
Reporter | ||
Comment 77•7 years ago
|
||
(In reply to Jeff Hajewski from comment #76) > This push to try is looking promising. Two failures that appear to be > intermittent failures (bug 1357082 and bug 1394982) but still a couple of > hours until the last test runs. If this is successful, I'll need to spend > some time figuring out why my local tests are failing... Sometimes, tests can fail locally for unrelated reasons (such as a difference between the local machine, and the machine used to run the test in automation). I wouldn't worry too much about it. If you'd like to be sure, you can run the tests locally without your patches applied. If that also fails, you know the failure is unrelated to your patches.
Reporter | ||
Comment 78•7 years ago
|
||
mozreview-review |
Comment on attachment 8904025 [details] Bug 1383816 - Fixes several minor errors in Variant's IPC implementation. https://reviewboard.mozilla.org/r/175752/#review182608 ::: gfx/layers/apz/src/FocusState.cpp:77 (Diff revision 5) > const FocusTarget& target = currentNode->second; > > // Accumulate event listener flags on the path to the focus target > mFocusHasKeyEventListeners |= target.mFocusHasKeyEventListeners; > > - switch (target.mType) { > + // Match on the data stored in mData Please document the meaning of the boolean return value of the match() methods. ::: gfx/layers/apz/src/FocusState.cpp:133 (Diff revision 5) > - return; > + return true; > } > - case FocusTarget::eNone: { > + }; // struct FocusTargetDataMatcher > - FS_LOG("Setting target to nil (reached a nil target)\n"); > > - // Mark what sequence number this target has for debugging purposes so > + if(target.mData.match(FocusTargetDataMatcher{*this, target.mSequenceNumber})) { nit: space between 'if' and '('
Reporter | ||
Comment 79•7 years ago
|
||
Finally, the "issues" created in MozReview for Ryan's comments need to be marked as fixed before the patches can be landed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 85•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #79) > Finally, the "issues" created in MozReview for Ryan's comments need to be > marked as fixed before the patches can be landed. Done, along with the fixes you requested in your latest review (comment 78).
Reporter | ||
Comment 86•7 years ago
|
||
The "Adds Variant in implementation of FocusTarget and removes union" patch is still marked as having "2 open issues" in MozReview. You need to close them before MozReview will allow me to land the patches. (I'd close them for you, but MozReview doesn't let me do that either :)).
Assignee | ||
Comment 87•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #86) > The "Adds Variant in implementation of FocusTarget and removes union" patch > is still marked as having "2 open issues" in MozReview. > > You need to close them before MozReview will allow me to land the patches. > (I'd close them for you, but MozReview doesn't let me do that either :)). I missed those -- still getting used to MozReview. I think they are all "fixed" now!
Comment 88•7 years ago
|
||
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 5 changesets with 7 changes to 5 files remote: remote: remote: ************************** ERROR **************************** remote: Rev 4bf9c7232c2e needs "Bug N" or "No bug" in the commit message. remote: Jeff Hajewski <jeff.hajewski@gmail.com> remote: Removes FocusTargetType ParamTraits specialization; r=botond remote: remote: MozReview-Commit-ID: Gao4O3eJKmp remote: ************************************************************* remote: remote: remote: remote: remote: ************************** ERROR **************************** remote: Rev 73a060e31b75 needs "Bug N" or "No bug" in the commit message. remote: Jeff Hajewski <jeff.hajewski@gmail.com> remote: Adds IPC Read and Write methods for FocusTarget and NoFocusState structs and creates EmptyStructSerializer helper class; r=botond remote: remote: Since NoFocusState is am empty struct used in the |mData| variant in remote: FocusTarget, we need to add a Reader and a Writer for IPC for NoFocusState so we remote: can properly read and write the |mData| variant. The NoFocusState Read and Write remote: methods do not read or write anything, since NoFocusState does not contain any remote: data. This is done by creating a helper class EmptyStructSerliazer and remote: inheritting from EmptyStructSerializer for the NoFocusState specialization. remote: remote: The |Read| and |Write| methods for FocusTarget are updated by removing the read remote: and write code for the individual types of |mData| and instead makes use of the remote: IPC read and write methods for Variant. remote: remote: MozReview-Commit-ID: 3159sp6FLek remote: ************************************************************* remote: remote: remote: remote: remote: ************************** ERROR **************************** remote: Rev 8b9fe7627131 needs "Bug N" or "No bug" in the commit message. remote: Jeff Hajewski <jeff.hajewski@gmail.com> remote: Updates FocusState to use variant matcher; r=botond remote: remote: Updates FocusState::Update to use a matcher on the the mData variant. This remote: replaces a switch statement that was used to unpack the mData union, prior to remote: making mData a variant. remote: remote: FocusTargetDataMatcher has three match methods, which are largely unmodified remote: from the three cases of the original switch statement. The constructor takes two remote: arguments: a reference to |this| (FocusState) and a copy of the remote: |sequenceNumber|. |sequenceNumber| was added to the constructor to avoid passing remote: in a reference to |target|, since we only needed the |sequenceNumber| property remote: from |target|. remote: remote: MozReview-Commit-ID: FkjQm8oGysM remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.c_commitmessage hook failed abort: push failed on remote
Assignee | ||
Comment 89•7 years ago
|
||
Does each commit message need to explicitly reference the bug? I wasn't sure about that since the commits were grouped... If so I will update later tonight around 5:30 Central.
Reporter | ||
Comment 90•7 years ago
|
||
Yeah, it looks like each commit does need to reference the bug number.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 96•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #90) > Yeah, it looks like each commit does need to reference the bug number. Added the bug number to all the commit messages for all the patches. Hopefully that goes through. I'm leaving shortly for a wedding so won't be able to make any more changes until Sunday evening (although can communicate here and via email).
Comment 97•7 years ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0e9b01271d17 Fixes several minor errors in Variant's IPC implementation. r=botond https://hg.mozilla.org/integration/autoland/rev/a61e73b332e1 Adds Variant in implementation of FocusTarget and removes union; r=botond https://hg.mozilla.org/integration/autoland/rev/7f209f4a6bf2 Updates FocusState to use variant matcher; r=botond https://hg.mozilla.org/integration/autoland/rev/ccd79eaa9cb0 Adds IPC Read and Write methods for FocusTarget and NoFocusState structs and creates EmptyStructSerializer helper class; r=botond https://hg.mozilla.org/integration/autoland/rev/57a5ce8cbf60 Removes FocusTargetType ParamTraits specialization; r=botond
Comment 98•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0e9b01271d17 https://hg.mozilla.org/mozilla-central/rev/a61e73b332e1 https://hg.mozilla.org/mozilla-central/rev/7f209f4a6bf2 https://hg.mozilla.org/mozilla-central/rev/ccd79eaa9cb0 https://hg.mozilla.org/mozilla-central/rev/57a5ce8cbf60
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter | ||
Comment 99•7 years ago
|
||
This has landed and stuck. Thanks for your work here, Jeff!
You need to log in
before you can comment on or make changes to this bug.
Description
•