Skip to content

Introduce an experiment to measure page jank via vsync.#7293

Merged
lannka merged 6 commits intoampproject:masterfrom
lannka:jank-counter
Feb 2, 2017
Merged

Introduce an experiment to measure page jank via vsync.#7293
lannka merged 6 commits intoampproject:masterfrom
lannka:jank-counter

Conversation

@lannka
Copy link
Copy Markdown
Contributor

@lannka lannka commented Feb 1, 2017

  • The experiment will display a jank meter at the left bottom of the page.
  • The experiment is purely opt-in based.

}

if (isExperimentOn(this.win, 'measure-jank') && this.win.performance) {
this.jankRateDisplay_ = this.win.document.createElement('div');
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.

Need to set this to null in the else.

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.

done

// Schedule actual animation frame and then run tasks.
this.scheduled_ = true;
if (this.jankRateDisplay_) {
this.scheduledTime_ = this.win.performance.now();
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.

this.scheduledTime_ needs to be added in the constructor.

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.

done

if (paintLatency > 100) {
this.bigJankCounter_++;
}
this.jankRateDisplay_./*OK*/innerText =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we use textContent instead

@lannka lannka merged commit 5b10381 into ampproject:master Feb 2, 2017
@lannka lannka deleted the jank-counter branch February 2, 2017 00:50
visibility: hidden;
}

.i-amphtml-pjr {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what does pjr stand for?

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.

page jank rate ...

if (paintLatency > 100) {
this.bigJankCounter_++;
}
this.jankRateDisplay_.textContent =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could add significant jank.

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.

why is that? i believe textContent does not re-layout.

Copy link
Copy Markdown
Member

@cramforce cramforce Feb 2, 2017

Choose a reason for hiding this comment

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

That is a bit of a misunderstanding. Reading it never layouts. That is the big (and unintuitive) difference from innerText. Writing it can obviously cause layout, since changing text can change layout.

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.

we are already in a vsync, so the impact should be minimum? since textContext doesn't take CSS into account, and will not do a re-measure and re-flow.

I know this is like quantum mechanics, measuring simply affects the thing it measures. But since this meter is opt-in only, so should be fine?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, its fine.

this.docState_.onVisibilityChanged(boundOnVisibilityChanged);
}

/** @private {?Element} */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please refactor this CL to isolate the jank rate specific functionality into a file or at least functions.

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.

will do

@lannka lannka mentioned this pull request Feb 6, 2017
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017
)

* Introduce an experiment to measure page jank via vsync.

* log info

* log info

* fix type checks

* pass presubmit check

* innerText -> textContent
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
)

* Introduce an experiment to measure page jank via vsync.

* log info

* log info

* fix type checks

* pass presubmit check

* innerText -> textContent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants