performance-impl: use performance.now() over Date.now()#26512
performance-impl: use performance.now() over Date.now()#26512samouri merged 9 commits intoampproject:masterfrom
Conversation
jridgewell
left a comment
There was a problem hiding this comment.
If I remember correctly, the reason we reverted the initial PR is because all of these values need to be normalized to an int. Date.now() always returns one, but performance.now() returns a float. We had some issue with our metrics because of the decimals.
src/service/performance-impl.js
Outdated
| */ | ||
| tick(label, opt_delta) { | ||
| const value = opt_delta == undefined ? this.win.Date.now() : undefined; | ||
| const value = opt_delta == undefined ? this.now() : undefined; |
There was a problem hiding this comment.
Can this be introduced in a backward-compatible way for existing AMP viewers that rely on the values being an absolute timestamp, as opposed to a time delta in the performance clock (relative to timeOrigin)?
IIUC, this change completely removes the current absolute timestamp value and replaces it with the performance clock value. Gmail's AMP viewer is currently relying on the value being absolute, so changing it to be relative would break Gmail's metrics. We raised the same concern in the reverted PR here, but we never got a response.
One way to introduce this in a backward-compatible way is to send both the absolute timestamp using Date.now() and the relative delta using the performance clock in the message posted to the viewer, in different properties of the message payload.
There was a problem hiding this comment.
Thank you for catching this! I was holding off on merging because I hadn't yet found the reason why the last one was reverted :). We can use performance.timeOrigin + performance.now() to keep the absolute timestamp.
There was a problem hiding this comment.
👏👏👏
Let's just switch this back to Date.now(), no need to be any more precise with it.
There was a problem hiding this comment.
Eh, performance.now() is still better than Date because of the clock skew issues right? Since we calculate diffs all over the place, its even a cleaner api to use (no longer need initTime_).
There was a problem hiding this comment.
Yeah, it's also us who requested for using the performance clock because our metrics currently suffer from clock skew. :P converting the relative timestamp in the performance clock to an absolute one SGTM since that should be backward compatible.
needs updates based on zhangsu's feedback
src/service/performance-impl.js
Outdated
| // We don't have the actual csi timer's clock start time, | ||
| // so we just have to use `docVisibleTime`. | ||
| this.prerenderComplete_(this.win.Date.now() - docVisibleTime); | ||
| // TODO before merge: what should be the actual value here? performance.now() or performance.now() - docVisibleTime? |
There was a problem hiding this comment.
I think it should be the latter. The code at HEAD passes this.win.Date.now() - docVisibleTime, which is the delta between two absolute dates. It's equivalent to the delta between two relative timestamps in the performance clock, so this.win.performance.now() - docVisibleTime in this PR.
There was a problem hiding this comment.
I tend to agree with you! The tests were getting away with counting docVisibleTime as 0, since they were never triggering the visible signal. I'll either update a test or add a new one and then submit this.
Fixes #16042
TODO: figure out why it was reverted last time (#16227).
Answer:
performance.now()is relative totimeOriginwhereasDate.now()is relative to epoch.