whatwg / dom

DOM Standard
https://dom.spec.whatwg.org/
Other
1.58k stars 294 forks source link

High resolution timing for events #23

Closed annevk closed 7 years ago

annevk commented 9 years ago

https://lists.w3.org/Archives/Public/public-web-perf/2014Jun/0013.html https://bugzilla.mozilla.org/show_bug.cgi?id=1026804

wycats commented 8 years ago

@birtles:

@wycats: @bzbarsky, @annevk and I discussed this in February this year and I think @bzbarsky in particular had a preference for adding another eventTime or time member given the compatibility concerns.

@majido:

I wish this was shared earlier. All other signals so far has been that Firefox is on-board with the plan to experiment and ship if we don't find a non-trivial compat issues.

TLDR: By February, multiple Mozilla engineers had become increasingly uncomfortable with the change and repeatedly described it as not-web-compatible.

The concerns about web compatibility were repeatedly shared earlier, around the same Feburary timeframe. Based on the threads you and @RByers participated in, I'm not sure where you are getting the idea that Firefox was still on board with the plan.


On December 10, in a linked issue from this thread:

We have a web compat issue reported in bug 1231619. See bug 1231619 comment 3.

In the referenced bug bug 1231619:

So this is an actual web compat problem.

At this point, Mozilla engineers were starting to think that there might be a web compat issue.

@RByers replied:

Personally I don't think this example alone should block our plans to ship DOM highRes event timestamps.

This is the first of many comments by Chrome engineers waving away reports of web compatibilities.

@birtles:

Clearing needinfo since it's clearly a Web compat problem.

@dbaron:

Web compatibility issue with a well-known library that we need to avoid shipping.

@RByers then tried to persuade Firefox to ship anyway. @dbaron replied:

Shipping a feature where a developer getting it wrong means their page is intermittently broken on a small percentage of machines seems pretty bad. Seems like people are likely to get that wrong forever. It seems better to actually go through a more-breaking change once.

Again, Mozilla engineers are becoming increasingly concerned, contrary to the idea that they were still just fine with the plan.

You replied, arguing that since Angular fixed the issue, we're clear to ship:

FYI, Angular has fixed the issue[1] and it is expected to be in their upcoming 1.5 release.

Around this time, @chancancode and I noticed that this problem broke one of our demos. @stefanpenner (another Ember developer) reported the "bug" to Chrome, and @chancancode elaborated:

Again, once we identified the problem, it was an easy fix, but I'm just quite worried about web-compat given the subtle nature of this difference.

@majido replied:

This is indeed the new desired behaviour which is currently planned to be launched on M49. See: https://www.chromestatus.com/features/5523910145605632

Again, no major concern that there might be a web compatibility issue, but rather "damn the torpedoes; full speed ahead!" and a suggestion that we should work harder to fix the problem on our end. We were happy to do so, but concerned about the web compat issue.

Since we quickly discovered the Angular thread and noticed that @majido and @RByers were on it, we moved the conversation there.

I believe there is a more general form of this problem that is likely to break other sites.

You replied:

I prefer that we try to use monotonic time first.

@chancancode replied with a very detailed comment explaining our concerns.

At this point, it appears that bz became increasingly concerned:

Fwiw, I generally agree with comment 19.

Around the same time that @chancancode and I reported the bug, YouTube engineers discovered a bug in their test suite.

The code in question was relatively simple:

var last = Date.now();
// later
video.addEventListener('timeupdate', function(e) {
  runner.checkGE(e.timeStamp, last, 'event.timeStamp');
  last = e.timeStamp;
});

In other words a classic example of wanting to compare an e.timeStamp against some earlier fixed time.

The YouTube engineer originally surmised:

This is a browser bug.

At this point, with the breakage in Angular, the report we had of a bug in a demo, and this bug by YouTube, it should have become clear that this change was not web-compatible. Instead of easing off and trying to learn more, the Chrome team pressed ahead, sure that they could just wish the problem away.

@majido told the YouTube engineer:

Sure, but there is a planned changed for DOM spec: https://github.com/whatwg/dom/issues/23 And Chrome M50 will reflect this change. As #16 pointed out the spec update is pending us successful shipment in Chrome.

The engineer replied:

Yes, I am the right owner to fix the test. This test is to ensure that browsers don't break YouTube. I'm going to wait until YouTube fixes their code before I fix the test. As of right now, the event.timeStamp epoch is still a requirement for YouTube.

Again, the fact that YouTube made this "mistake" should have been enough evidence to ease off and get more information.

Now that Chrome has shipped, you're saying things like:

On YouTube case: They had an existing workaround for Firefox edgecases which works well for Chrome recent changes as well. So they haven't done any modification in response to a breakage. So there was no modification or impact.

I can't see the YouTube source code, but after Chrome shipped, YouTube did update the test in question to stop relying on e.timeStamp being directly comparable to Date.now() with the message "Updated EventTimestamp test on Tip to be more spec compliant and Chrome compliant".

I'm not sure where you got the idea that everyone was just fine with moving ahead, but it does seem clear that you deeply wished for that to be true.

It continues to be the case that textual analysis is very unlikely to identify most sources of this bug, and that incidents of the bug are likely to be under-reported (people will believe that the web is janky, rather than that there is a bug in the content they're looking at).

bzbarsky commented 8 years ago

One point of record: @dbaron's quote above wrt to a more-breaking change was a response to a suggestion that we define event.timeStamp in terms of performance.now() + navigationStart, more or less, so it looks comparable to Date.now() values. But isn't really, as clocks drift during the lifetime of the page. He was saying he would prefer just having event.timeStamp not be comparable to Date.now() at all to that setup.

RByers commented 8 years ago

Ok let's take the rhetoric down a notch, we're all trying to do the best thing for the web platform.

There was general agreement that (in order to resolve the multi-year debate about whether a new API is necessary or not) Chrome should attempt to ship this change. Once we started down that path and were getting concrete data, our judgement (vocal opposition notwithstanding) was that the compat issues we were seeing were minor and the short-term cost caused was well below the long-term cost to the platform of introducing a confusing and largely redundant API. So we did not see a need to reverse course. But I recognize this is entirely a judgement call where opinions of reasonable engineers will differ.

If Mozilla and Apple now feel that, despite not having concrete examples of real sites that are broken today in Chrome, there still needs to be a new API for this, then by all means please spec such an API, ship it in at least one of your engines, and we'll be happy to follow suit. We really do want to see the problem of reliably measuring input event latency solved once and for all in an interoperable way.

annevk commented 8 years ago

It seems that from https://github.com/whatwg/dom/issues/23#issuecomment-185920481 the current requirement in DOM for timeStamp is not onerous and can be implemented across browsers.

It's not clear if we want to store two different times for each event, forever (different in both origin and resolution). (Though Apple seems okay with this given @rniwa's comment.)

It's also not clear what the compatibility impact is of changing timeStamp, exactly, though with Chrome shipping the change it cannot be huge, I think. Though as suggested earlier we might know more in a couple of months.

It seems unlikely other vendors will ship something fast, given https://github.com/whatwg/dom/issues/23#issuecomment-213079387 and https://github.com/whatwg/dom/issues/23#issuecomment-215242830, so maybe the best approach here is to just wait it out a bit and see what actually happens.

RByers commented 8 years ago

It seems unlikely other vendors will ship something fast, given #23 (comment) and #23 (comment), maybe the best approach here is to just wait it out a bit and see what actually happens.

Actually @rniwa didn't say anything about the priority they'd give to landing a new API in WebKit here. @rniwa thoughts?

Note that with passive event listeners now shipping, we're starting to actively encourage the measurement of scroll latency using this API. But all our examples will show a pattern that works for both the WebKit and blink implementations (the only two implementations today exposing the original driver time), so it'll probably still be possible for us to switch back to the WebKit behavior in the future without causing too much disruption (it'll just introduce the NTP skew problem in scenarios that were previously more stable in Chrome, but we don't know how bad that tends to be in practice).

So if collecting more data on the impact in Chrome over a few months would be valuable to this debate, then that's fine with me. But if no amount of such additional data would change other implementor's minds then there's no benefit to waiting.

wycats commented 8 years ago

Ok let's take the rhetoric down a notch, we're all trying to do the best thing for the web platform.

I apologize for the rhetoric, and I agree fully that everyone here is trying to do what they believe is best for the platform and the browsers they work on.

So if collecting more data on the impact in Chrome over a few months would be valuable to this debate, then that's fine with me.

I think more data would indeed be useful, if we measure the right things.

Here are some things that (dynamic) telemetry might be helpful if unearthing, if you're willing to run it:

I'd be happy to discuss the exact telemetry that would be most helpful here, if you find this line of reasoning useful.

I'd also add that finding "hits" with this kind of telemetry does not prove that a site is broken, but it will give us a corpus of sites to evaluate manually.

Personally, I'm especially concerned about the vast number of "iOS lookalike" sites that were hand-crafted early in the days of iOS, because those sites were forced to reimplement scrolling, implement high-level touch events, implement animations and more, without the benefit of window.performance.now() even existing at all to give them a clue that they should avoid comparing e.timeStamp with Date.now().

Also, because those sites are largely "webkit only" in practice, they may well not have noticed the Firefox breakage at all, but these sites are on the Internet and most browsers have been attempting to support them.

dmethvin commented 8 years ago

Just some data, event.timeStamp has been a PITA to jQuery for a long time since people expected us to fix it. It was broken in Firefox until 2014 and broken in Opera until 2012. We also received a report in 2012 that it was broken on Android 2.3 Dolphin. So anyone depending on reliable cross-browser results has been getting wrong answers until the last 3 or 4 years.

jQuery used to try patching this problem by filling in a Date.now() value for the timestamp if it was zero but removed that code in jQuery 1.7 (January 2011). I hope that encouraged people to stop using the value altogether, but you can still find various GitHub code and StackOverflow answers that assume the timestamp is compatible with Date.now().

wycats commented 8 years ago

I had a good chat with @majido off-thread and we came up with some good telemetry that we agree would identify the impact of this change. I'm looking forward to the results!

majido commented 8 years ago

It was a good chat indeed. :)

I am trying to see if I can gather more data using telemetry to better help make progress on the debate about the potential impact. This should help address some cases where our earlier static analysis of httparchive would have missed. I can only do this mainly thanks to @RByers recent efforts to enable cluster telemetry for Blink that allows such measurement for a set of top sites (e.g, top 10K).

Following @wycats suggestions I think we should measure the following cases:

  1. anyDate - timestamp or timestamp - anyDate
  2. new Date(timestamp)
  3. date.setTime(timestamp)

Where timestamp is a value that came from Event.timeStamp **

As pointed out earlier, this will over count because there is no guarantee that any of the operations above will actually lead to breakage but we can followup with a manual check on this smaller subset. I am discussing with our V8 experts to see if this is actually possible without a huge amount of work. I will update once I have an answer.

In any case, before spending too much effort on this I like to have some agreement that this is an acceptable experiments and an acceptable threshold (e.g., 0.02% of pages?) for seeing this type of usage leading to breakage.

Also particularly interested on what @rniwa think of the effectiveness of this measurements given his strong concerns on web-compat.

\ I have not yet figured exactly how to tag timestamp and Date values as I believe they get converted into a numbers before being operated on. Perhaps I will use a special value (may impact program flow) or specific fractions (A lot less intrusive)

wycats commented 8 years ago

In any case, before spending too much effort on this I like to have some agreement that this is an acceptable experiments and an acceptable threshold (e.g., 0.02% of pages?) for seeing this type of usage leading to breakage.

Generally speaking, I have concerns about treating any specific percentage of pages as evidence that breakage is ok. Because the web is so huge, "real" applications are a very small percentage. Maybe we could categorize breakage by things like presence of certain globals (Ember, Backbone, React), usage of other "tracer" features like history.pushState that would indicate that a website is more of an "application" than pure content?

If you're interested, I would love to help beef up Blink's usage analysis in this way ("application" is one useful category, but there are probably others, like "mobile site", etc.). It would make me feel a lot more confident that the identified features aren't heavily used in some pocket of the web.

wycats commented 8 years ago

But I would also like to add that I think we can come up with a measurement that I could agree ahead of time would be enough (just not "total usage over the entire web").

majido commented 8 years ago

But I would also like to add that I think we can come up with a measurement that I could agree ahead of time would be enough (just not "total usage over the entire web").

If I use cluster telemetry, the corpus will be pretty well defined (e.g. top 10K sites) rather than the entire web and I can better control for mobile vs desktop. Though it also means I can only test a pre-defined set of interactions but has the benefit of being able to get the URL of the page.

If I end up being able to get some type of safe measurement in Canary then it will be website used by Chrome Canary users. This is more like the entire web but page URL will not be available.

RByers commented 8 years ago

If you're interested, I would love to help beef up Blink's usage analysis in this way ("application" is one useful category, but there are probably others, like "mobile site", etc.). It would make me feel a lot more confident that the identified features aren't heavily used in some pocket of the web.

Yeah this is a problem the blink API owners (especially @foolip and I) have spent some time worrying about, but don't yet have any super-promising practical ideas around. We should think about it further and we'd love your help! We're ramping up our investments in our compat tooling, deprecation process, interop efforts, etc. I filed this chromium bug to track.

majido commented 8 years ago

@dmethvin Thanks for the jQuery background. Current Event.timeStamp is PITA indeed. Hopefully we get it fixed one way or another soon.

Even today despite my experience using Event.timeStamp I got surprised seeing negative values (up to -300ms) for Date.now() - event.timeStamp for input events in Safari on Mac OS (demo page) /cc @rniwa. It is a small issue (most probably due to an NTP skew) compared to others but can lead to subtle head scratching bugs.

RByers commented 8 years ago

There was some discussion of this at TPAC:

SebastianZ commented 7 years ago

FYI, bug 1026804 got fixed two days ago.

Sebastian

cpeterso commented 7 years ago

After almost three years of testing with Firefox Nightly and Dev Edition users, Mozilla plans to ship high-resolution Event.timeStamp in Firefox 54 (2017-06-13). See @birtles' Intent to Ship announcement.

annevk commented 7 years ago

Okay, so with Chrome and Firefox shipping, these things still need to be done:

I'm happy to help folks if they want to take on one or more of these tasks.

majido commented 7 years ago
annevk commented 7 years ago

Edge: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12726672/