Skip to content

Conversation

@jtmcdole
Copy link
Member

Initial tap is missing sometimes; either its never delivered or it is delivered before gesture controller is hooked up.

1: Update the two perf tests to output when TAPPED is received
2: Update the MemoryTest to keep tapping while waiting for TAPPED

Tested on devicelab:

  • setting iterations=1
  • removing the timeout before READY
  • running tests in a while loop

Before this change, you could get the test to hang often. After this change you'll see "tapping device... [x]" where x is the counter.

Fixes #150096

Initial tap is missing sometimes; either its never delivered or it is
delivered before gesture controller is hooked up.

1: Update the two perf tests to output when TAPPED is received
2: Update the MemoryTest to keep tapping while waiting for TAPPED

Tested on devicelab:
* setting iterations=1
* removing the timeout before READY
* running tests in a while loop

Before this change, you could get the test to hang often. After this
change you'll see "tapping device... [x]" where x is the counter.

Fixes flutter#150096
@jtmcdole jtmcdole requested a review from gaaclarke June 14, 2024 23:27
@github-actions github-actions bot added the f: scrolling Viewports, list views, slivers, etc. label Jun 14, 2024
@jtmcdole jtmcdole added team-infra Owned by Infrastructure team and removed f: scrolling Viewports, list views, slivers, etc. labels Jun 14, 2024
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm modulo single character variable name.

Instead of repeatedly tapping, it seems like you could do the same thing you did to receive the tap event to know when the thing is ready to be tapped. Seems like there should be a latch for that in Flutter? Maybe sending the message when the State is initialized would be enough?

There is a risk of accidentally tapping twice before registering the tap because of some sort of latency in receiving the TAPPED print statement. Hopefully 250ms makes that unlikely though and is definitely better than what we had.

@github-actions github-actions bot added f: scrolling Viewports, list views, slivers, etc. and removed team-infra Owned by Infrastructure team labels Jun 14, 2024
@jtmcdole
Copy link
Member Author

Instead of repeatedly tapping, it seems like you could do the same thing you did to receive the tap event to know when the thing is ready to be tapped. Seems like there should be a latch for that in Flutter? Maybe sending the message when the State is initialized would be enough?

I think that would be a moving target. If there was a magical "we're ready for input" then I'd wait on it; but a side effect of state loaded might be undocumented.

There is a risk of accidentally tapping twice before registering the tap because of some sort of latency in receiving the TAPPED print statement. Hopefully 250ms makes that unlikely though and is definitely better than what we had.

I could increase the caffeine to 100ms tapping if that makes things better. We could also delay after the READY for ~milliseconds.

- Wait out any errant taps (~200ms)
- Update i->tapCount
@jtmcdole jtmcdole added autosubmit Merge PR when tree becomes green via auto submit App fyi-infra For the attention of Infrastructure team labels Jun 15, 2024
@auto-submit auto-submit bot merged commit 499c84c into flutter:master Jun 15, 2024
@jtmcdole jtmcdole added the revert Autorevert PR (with "Reason for revert:" comment) label Jun 15, 2024
@jtmcdole
Copy link
Member Author

The other tests don't send the "TAPPED" signal, so of course they timeout.

@auto-submit
Copy link
Contributor

auto-submit bot commented Jun 15, 2024

A reason for requesting a revert of flutter/flutter/150287 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Jun 15, 2024
@jtmcdole
Copy link
Member Author

Reason for revert: other memperf tests don't wait for or send a TAPPED; so they fail.

@jtmcdole jtmcdole added the revert Autorevert PR (with "Reason for revert:" comment) label Jun 15, 2024
auto-submit bot pushed a commit that referenced this pull request Jun 15, 2024
@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Jun 15, 2024
auto-submit bot added a commit that referenced this pull request Jun 15, 2024
…#150293)

Reverts: #150287
Initiated by: jtmcdole
Reason for reverting: other memperf tests don't wait for or send a TAPPED; so they fail.
Original PR Author: jtmcdole

Reviewed By: {gaaclarke}

This change reverts the following previous change:
Initial tap is missing sometimes; either its never delivered or it is delivered before gesture controller is hooked up.

1: Update the two perf tests to output when TAPPED is received
2: Update the MemoryTest to keep tapping while waiting for TAPPED

Tested on devicelab:
* setting iterations=1
* removing the timeout before READY
* running tests in a while loop

Before this change, you could get the test to hang often. After this change you'll see "tapping device... [x]" where x is the counter.

Fixes #150096
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 15, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 15, 2024
flutter/flutter@349ec71...5187cab

2024-06-15 [email protected] Roll Flutter Engine from 8167dffd1914 to 9779c273aac3 (21 revisions) (flutter/flutter#150290)
2024-06-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix flaky complex_layout_scroll_perf__memory test (#150287)" (flutter/flutter#150293)
2024-06-15 [email protected] Fix flaky complex_layout_scroll_perf__memory test (flutter/flutter#150287)
2024-06-14 [email protected] Make `CupertinoTextField` respect decoration color when disabled (flutter/flutter#149774)
2024-06-14 [email protected] Add transparent color to Cupertino colors (flutter/flutter#150149)
2024-06-14 [email protected] Roll pub packages (flutter/flutter#150267)
2024-06-14 [email protected] Roll Packages from 7805455 to dd04ab1 (4 revisions) (flutter/flutter#150264)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@jtmcdole jtmcdole deleted the fixFlakeyMemPerf branch June 17, 2024 18:28
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. fyi-infra For the attention of Infrastructure team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linux_mokey complex_layout_scroll_perf__memory is 2.44% flaky

2 participants