1380003 - Prevent using Date.now() under testing/talos via eslint plugin

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

[Andrew Halberstadt [:ahal]]

In bug 1372261, mconley discovered that results were noisy due to clock skew from using Date.now() for timing. Now, there is a new (more precise) timing API under window.performance.

To get the current milliseconds since the epoch, we can use:
performance.timing.navigationStart + performance.now()

Someone suggested this would make a good lint rule, especially in a perf framework like Talos. There are 24 current cases of using Date to grab current time (some of which are being fixed in bug 1372261).

We could enable the lint rule and disable it via comments for the remaining instances for now (and file a follow-up bug to fix them). This way we'd prevent future use of the Date api.

I have the lint rule written locally. It only prevents Date.now() and new Date() without arguments. It wouldn't stop e.g, new Date('2017-01-01') as that is obviously not being used for timing purposes.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1380003 - Create avoid-Date-timing eslint rule,

Review commit: https://reviewboard.mozilla.org/r/156600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156600/

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1380003 - Enable avoid-Date-timing eslint rule on testing/talos,

Review commit: https://reviewboard.mozilla.org/r/156602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156602/

Comment 3

[Andrew Halberstadt [:ahal]]

There are still perfectly valid uses of Date.now(). E.g creating random numbers or for log timestamps with low resolution (e.g seconds). So I only anticipate we'd be enabling this rule in modules where high res timing is important, like perf frameworks.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8885820 [details]
Bug 1380003 - Enable avoid-Date-timing eslint rule on testing/talos,

https://reviewboard.mozilla.org/r/156602/#review161736

a couple of possible oversights, but otherwise this looks good.

::: testing/talos/talos/tests/a11y/dhtml.html:20
(Diff revision 1)
>        return;
>      }
>  
>      var container = document.getElementById("container");
>      var lastchild = document.getElementById("lastchild");
> -    var start = new Date();
> +    var start = new Date();  // eslint-disable-line mozilla/avoid-Date-timing

shouldn't this be performance.now?

::: testing/talos/talos/tests/a11y/dhtml.html:46
(Diff revision 1)
>      setTimeout(postProcessingRecord, 0, start);
>    }
>  
>    function postProcessingRecord(s) {
>      // alert(new Date() - s);
> -    tpRecordTime(new Date() - s, s);
> +    tpRecordTime(new Date() - s, s);  // eslint-disable-line mozilla/avoid-Date-timing

shouldn't this be performance.now?

::: testing/talos/talos/tests/a11y/tablemutation.html:133
(Diff revision 1)
>    setTimeout(postProcessingRecord, 0, start);
>  }
>  
>  function postProcessingRecord(s) {
>    // alert(new Date() - s);
> -  tpRecordTime(new Date() - s, s);
> +  tpRecordTime(new Date() - s, s); // eslint-disable-line mozilla/avoid-Date-timing

shouldn't this be performance.now?

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8885819 [details]
Bug 1380003 - Create avoid-Date-timing eslint rule,

https://reviewboard.mozilla.org/r/156600/#review161984

Looks good. Thanks for adding the tests.

Comment 6

[Andrew Halberstadt [:ahal]]

Mike, I meant to ask, do you know of any other places in the tree we might want to enable this rule?

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8885820 [details]
Bug 1380003 - Enable avoid-Date-timing eslint rule on testing/talos,

https://reviewboard.mozilla.org/r/156602/#review161736

> shouldn't this be performance.now?

This patch isn't actually fixing the bugs, it's just disabling the eslint rule on these lines. I filed bug 1380676 to tackle actually fixing them. I wanted to split this up because I'm not familiar with this code, so I'd want to do much more thorough testing before making those changes in case I accidentally broke some subtle thing.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrew Halberstadt [:ahal] from comment #6)
> Mike, I meant to ask, do you know of any other places in the tree we might
> want to enable this rule?

Good question.

I suspect we should never use Date.now() to make any kind of decision about when something has occurred. It's probably fine to use it to seed the display of, for example, our datetime popups... but anytime it's being used by backend code to make business decisions ("A is clearly older than B"), I think Date.now() isn't the right choice.

I'm honestly not sure how we turn that kind of distinction into an eslint rule though. :/

Perhaps eoger has some suggestions on this? We were talking about this class of problems earlier this week, and I think the Sync team dealt with a similar problem. Perhaps he has an idea of a concrete class of usage that we can blacklist.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8885819 [details]
Bug 1380003 - Create avoid-Date-timing eslint rule,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156600/diff/1-2/

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8885820 [details]
Bug 1380003 - Enable avoid-Date-timing eslint rule on testing/talos,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156602/diff/1-2/

Comment 11

[Edouard Oger [:eoger]]

I think we should be fine, our clock skew problems come from the difference between the server and the local client.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6daff898c07f
Create avoid-Date-timing eslint rule, r=standard8
https://hg.mozilla.org/integration/autoland/rev/425e84ac0954
Enable avoid-Date-timing eslint rule on testing/talos, r=jmaher

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6daff898c07f
https://hg.mozilla.org/mozilla-central/rev/425e84ac0954

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

this patch has an improvement in talos numbers:
== Change summary for alert #7966 (as of July 14 2017 00:12 UTC) ==

Improvements:

  2%  tp5n main_startup_fileio windows7-32 opt e10s     69,966,987.00 -> 68,539,475.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7966