Closed Bug 1514513 Opened 1 year ago Closed 1 year ago

TTFI calculations aren't correct in some cases

Categories

(Core :: Gecko Profiler, enhancement, P2)

57 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(1 file, 1 obsolete file)

The TTFI calculation fails in a few cases, especially when there's no 50ms MainThread hang after FCP, but also the calculation of the end where DCL and longTask are used is wrong (the spec proposals are written in a muddy manner).
Added PageLoad log category as well
Attachment #9031712 - Flags: review?(mstange)
Comment on attachment 9031712 [details] [diff] [review]
Fix TTFI calculations and match proposed spec

Review of attachment 9031712 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMNavigationTiming.cpp
@@ +300,5 @@
> +      lastLongTaskEnded > mDOMContentLoadedEventEnd) ?
> +      lastLongTaskEnded : mDOMContentLoadedEventEnd;
> +    PAGELOAD_LOG(("mTTFI (task: %g, DCL: %g): %g",
> +        (lastLongTaskEnded-mNavigationStart).ToMilliseconds(),
> +        (mDOMContentLoadedEventEnd-mNavigationStart).ToMilliseconds(),

This subtraction is outside of a mDOMContentLoadedEventEnd.IsNull() check, so I think it can assert.

Don't forget to run mach clang-format on your patch.
Attachment #9031712 - Flags: review?(mstange) → review+
And I wonder if the log category should be called "DOMNavigationTiming" instead. PageLoad is a really generic name.
reworked the log/etc, re-asking for review (should be simple)
Attachment #9032026 - Flags: review?(mstange)
Attachment #9031712 - Attachment is obsolete: true
Comment on attachment 9032026 [details] [diff] [review]
Fix TTFI calculations and match proposed spec

Review of attachment 9032026 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but it'll need more changes if you switch away from TextMarkerPayload as requested in the other bug.
Attachment #9032026 - Flags: review?(mstange) → review+
> Looks good but it'll need more changes if you switch away from
> TextMarkerPayload as requested in the other bug.

This patch is written assuming bug 1508837 is landed (switch from UserTiming markers to (new) TextMarkers)
Depends on: 1508837
Priority: -- → P2
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e75d5ad3de
Fix TTFI calculations and match proposed spec r=mstange
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Depends on: 1520451
You need to log in before you can comment on or make changes to this bug.