Skip to content

Add benchmark reproducing large static scrolling content#53686

Merged
fluttergithubbot merged 2 commits intoflutter:masterfrom
yjbanov:bench-dynamic-clip
Apr 1, 2020
Merged

Add benchmark reproducing large static scrolling content#53686
fluttergithubbot merged 2 commits intoflutter:masterfrom
yjbanov:bench-dynamic-clip

Conversation

@yjbanov
Copy link
Copy Markdown
Contributor

@yjbanov yjbanov commented Mar 31, 2020

Description

Add benchmark that reproduces #42987, i.e. a large piece of static content (a Picture that never changes) is scrolled. As the clip slides along the picture it causes repaints. Since our DOM operations are not culled, it causes jank proportional to the size of the picture.

I will send a separate fix in flutter/engine, but want to get the benchmark first so we can validate the fix later.

Related Issues

#42987

@yjbanov yjbanov requested review from ferhatb and mdebbar March 31, 2020 20:32
@fluttergithubbot
Copy link
Copy Markdown
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Mar 31, 2020
Copy link
Copy Markdown
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM

import 'recorder.dart';
import 'test_data.dart';

/// The number of rows should be such that
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

such that ... ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops. Leftover. I moved this check into the constructor. Replaced with dartdocs.

Comment on lines +33 to +34
const double maxScrollExtent = kMaxSampleCount * kScrollDelta;
const double pictureHeight = kRows * kRowHeight;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious why these constants are here instead of at the top with the others?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They are here for validation only, and they don't supply any new configuration for this benchmark. The ones at the top can be manually tweaked, unlike these that will always remain derivatives of the top-level constants.

Copy link
Copy Markdown
Contributor

@ferhatb ferhatb left a comment

Choose a reason for hiding this comment

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

Other than comment /// The number of rows should be such that , lgtm

@fluttergithubbot fluttergithubbot merged commit 0d07788 into flutter:master Apr 1, 2020
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants