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)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: botond, Assigned: jeff.hajewski, Mentored)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 1 obsolete file)

Bug 1383816 - Adds Variant in implementation of FocusTarget and removes union;
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1383816 - Updates FocusState to use variant matcher;
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1383816 - Adds IPC Read and Write methods for FocusTarget and NoFocusState structs and creates EmptyStructSerializer helper class;
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1383816 - Fixes several minor errors in Variant's IPC implementation.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1383816 - Removes FocusTargetType ParamTraits specialization;
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
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.
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
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...
(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 :)
(In reply to Botond Ballo [:botond] from comment #4)

Thanks for following up on this Botond! I'll keep plugging away
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`.
(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`
(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.
+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.
(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.
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
(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.
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
(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.
Assignee: nobody → jeff.hajewski
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.
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)
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")
(More reviews to come later today.)
(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!
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)
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)
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)
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)
Comment on attachment 8904026 [details]
Bug 1383816 - Removes FocusTargetType ParamTraits specialization;

https://reviewboard.mozilla.org/r/175762/#review180804
Attachment #8904026 - Flags: review?(botond) → 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)
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.
Attachment #8904027 - Attachment is obsolete: true
(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.
(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.
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)
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+
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+
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+
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 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+
(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...
(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
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.
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.
(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).
(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 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.)
(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
(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.
(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 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 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 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
Sorry about the drive by reviews, I saw there were some interesting test failures and wanted to take a look. :)
(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.
(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.
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.
(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/
(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
(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])?
(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...
(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.
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 '('
Finally, the "issues" created in MozReview for Ryan's comments need to be marked as fixed before the patches can be landed.
(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).
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 :)).
(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!
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
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.
Yeah, it looks like each commit does need to reference the bug number.
(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).
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: