Use performance.now over Date.now in performance metrics by alabiaga · Pull Request #16119 · ampproject/amphtml

alabiaga · 2018-06-18T20:46:48Z

use performance.now in metrics
fix missing jsdocs in performance-impl
fix corresponding return types in url-replacements-impl due to jsdoc doc change
update amp-access tests to polyfill performance.now as it is not present in a fakeWin
update amp-access test to remove pubOrigin expect for 'http' as the pubOrigin for the iframe is now about:src since the fakeLocation is on win.testLocation as opposed to win.location when setting, as win.location on an iframe is not configurable. see describes.js#setup(env)
remove opt_disableFallback param in amp-access.js#runAuthorization_() method as it is not used and remove related tests. This change was required as one of the test was failing, and not due to my changes as it fails on master.

dreamofabear · 2018-06-19T16:41:55Z

extensions/amp-access/0.1/test/test-amp-access.js



-describes.fakeWin('AccessService multiple sources', {
+describes.realWin('AccessService multiple sources', {


Why the changes in this file?

amp-access tests were failing because performance is not present in a fakeWin

alabiaga · 2018-06-19T20:33:57Z

PTAL. updated broken test in amp-access and polyfilled performance.now as opposed to using realWin. Thanks

dreamofabear · 2018-06-19T21:17:53Z

extensions/amp-access/0.1/test/test-amp-access.js

-
+    // Polyfill performance.now() method as it is not available in fakeWin
+    // environment.
+    win.performance = {now: () => Date.now()};


You could also just add it to fake-dom.js#FakeWindow.

done. thanks!

Looks like there are still a bunch of these in this file.

right..my editor cache needed to be cleared..for some reason it was showing my updated changes but this file wasn't actually edited. Fixed.

kristoferbaxter · 2018-06-19T22:16:18Z

extensions/amp-access/0.1/test/test-amp-access.js

-
+    // Polyfill performance.now() method as it is not available in fakeWin
+    // environment.
+    win.performance = {now: () => Date.now()};


Since it ended up needing to be polyfilled in so many places, does it make sense to move this polyfill to env.win so each test doesn't need to repeat the polyfill?

Thanks for your feedback Kris. Will actually commented on that as well and I made the changes and moved this to fake-dom.js

dreamofabear · 2018-06-20T15:25:06Z

testing/fake-dom.js

    });
+    // Polyfill performance.now() method as it would normally not be available
+    // in a non document environment. Used for tests using fakeWin that rely on
+    // the performance-impl service.


This comment is slightly out of context and can be updated.

what would you suggest?

How about just // Polyfill performance.now with Date.now.. I think most of the current comment is obvious since it's living in FakeWindow.

dreamofabear · 2018-06-20T15:25:37Z

testing/fake-dom.js

-   * @param {function(!Event)} handler
-   * @param {(boolean|!Object)=} captureOrOpts
-   */
+  /** polyfill addEventListener. */


Why replace the type declarations? Were they causing errors?

yes lint errors

I think you can just remove these "polyfill X" comments. They don't add much.

dreamofabear

LGTM, just some nits.

dreamofabear · 2018-06-20T21:58:51Z

testing/fake-dom.js

-   * @param {function(!Event)} handler
-   * @param {(boolean|!Object)=} captureOrOpts
-   */
+  /** polyfill addEventListener. */


I think you can just remove these "polyfill X" comments. They don't add much.

dreamofabear · 2018-06-20T22:11:01Z

testing/fake-dom.js

    });
+    // Polyfill performance.now() method as it would normally not be available
+    // in a non document environment. Used for tests using fakeWin that rely on
+    // the performance-impl service.


How about just // Polyfill performance.now with Date.now.. I think most of the current comment is obvious since it's living in FakeWindow.

dreamofabear · 2018-06-20T22:12:27Z

testing/fake-dom.js

    /** @const */
    this.Date = window.Date;

+    this.setTimeout = function() {


The JSDoc below doesn't seem useful when switching the order like this. We can either add the missing params (prefixed with unused) or remove the JSDoc outright. I think it's fine to remove since they're just standard window APIs.

Ditto below.

done. Thanks

mattpk · 2018-06-21T18:47:48Z

@choumx @alabiaga @zhangsu @raywainman

Thanks for addressing the bug so quickly. However, is it okay to just replace Date.now() with performance.now()? Performance.now returns a timestamp relative to performance.timing.navigationStart, which would be orders of magnitude different from Date.now()'s epoch time. Would this break any clients expecting the tick events to provide absolute timestamps?

A potential solution could be to return performance.now() + performance.timing.navigationStart so that the timestamp is still absolute. Alternatively, would it be possible to send the performance.timing.navigationStart value to the client so that we can know what the performance.now() timestamps are relative to? I can file an issue with a request if that sounds feasible.

jridgewell · 2018-06-21T18:53:18Z

src/service/performance-impl.js


      // Tick the "messaging ready" signal.
-      this.tickDelta('msr', this.win.Date.now() - this.initTime_);
+      this.tickDelta('msr', this.win.performance.now() - this.initTime_);


Oh man, this needs to be normalized to an int.

i'll follow up in a separate PR. good point.

)" This reverts commit 0d2b567.

…6227) * Revert "Revert "Update dependencies for default 🌴 (#16221)" This reverts commit f8da39e. * Revert "Mark 4 more visual diff tests as flaky (#16214)" This reverts commit 867e903. * Revert "Update dependencies for default 🌴 (#16195)" This reverts commit 34d1977. * Revert "Fix selector (#16197)" This reverts commit f741868. * Revert "🐛 Position offscreen amp-story-pages using viewport widths (#16092)" This reverts commit fa85965. * Revert "Use performance.now over Date.now in performance metrics (#16119)" This reverts commit 0d2b567. * fix lint errors * fix lint error in constructor * Update performance-impl.js fix more lint errors * Update url-replacements-impl.js more lint.. * Update test-amp-access.js fix test error. * Update test-amp-access-source.js fix more tests...

googlebot added the cla: yes label Jun 18, 2018

alabiaga changed the title ~~Use performance.now over Date.now~~ Use performance.now over Date.now in performance metrics Jun 18, 2018

alabiaga requested review from dreamofabear and jridgewell June 18, 2018 22:00

jridgewell approved these changes Jun 18, 2018

View reviewed changes

dreamofabear reviewed Jun 19, 2018

View reviewed changes

kristoferbaxter reviewed Jun 19, 2018

View reviewed changes

alabiaga added 14 commits June 20, 2018 10:15

use performance.now vs Date.now

7ecfdf4

fix test

e44ded2

fix lint errors

579b8e5

fix comment

3fb56f2

fix typo

1f31f67

fix comment again

da8abb3

fix comment again

a21865b

fix stuff

8a4d649

fix amp-access test

9b5110d

fix tests

5f3e291

fixes

9517f7b

polyfill performance.now as opposed to using realWin

0771c2b

port polyfill to fake-dom

6c186f7

fix lint errors on fake-dom

8b3a11c

dreamofabear reviewed Jun 20, 2018

View reviewed changes

fix test amp-access

b3b1db5

dreamofabear approved these changes Jun 20, 2018

View reviewed changes

address @choumx comments

fb38a6c

alabiaga merged commit 0d2b567 into ampproject:master Jun 21, 2018

alabiaga deleted the 16042 branch June 21, 2018 14:32

jridgewell reviewed Jun 21, 2018

View reviewed changes

jridgewell mentioned this pull request Jun 21, 2018

normalize DomHighResTimeStamp to an int #16224

Closed

alabiaga added a commit that referenced this pull request Jun 21, 2018

Revert "Use performance.now over Date.now in performance metrics (#16119

a7b5827

)" This reverts commit 0d2b567.

alabiaga mentioned this pull request Jun 21, 2018

Revert "Use performance.now over Date.now in performance metrics" #16227

Merged

zhangsu mentioned this pull request Jan 30, 2020

performance-impl: use performance.now() over Date.now() #26512

Merged



		describes.fakeWin('AccessService multiple sources', {
		describes.realWin('AccessService multiple sources', {

Conversation

alabiaga commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alabiaga Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alabiaga commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpk commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alabiaga commented Jun 18, 2018 •

edited

Loading

alabiaga Jun 19, 2018 •

edited

Loading