Closed Bug 1508837 Opened Last year Closed 11 months ago

TTI marker incorrectly uses the UserTiming payload

Categories

(Core :: Gecko Profiler, enhancement, P3)

62 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: gregtatum, Assigned: jesup)

References

Details

Attachments

(1 file, 1 obsolete file)

https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/dom/base/nsDOMNavigationTiming.cpp#368

The payload for UserTimingMarkerPayload should be used for the UserTiming API for performance.mark() and performance.measure(). It will be incorrectly categorized in the profiler front-end using this payload.
Priority: -- → P3
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
(In reply to Randell Jesup [:jesup] from comment #1)
> Created attachment 9031710 [details] [diff] [review]
> Add TextMarker payloads for simple Profiler Markers

We do handle some "text only" markers pre-existing perf.html, but want to move away from them, and we certainly don't want to add more of them as they're hard to parse and easy to break. We definitely want to use structured payloads. Is there a compelling reason to use a text marker her?

(basing my comment on the output you mentioned in bug 1514235 comment 8).
I agree with Julien; I'd be much happier with a patch that gave these markers a payload with a proper data object, with a "foregroundTab" field and maybe a "elapsedTimeSinceNavigationStart" field.
The marker names seem fine.
Blocks: 1517272
Blocks: 1518030
added some alternate constructors
Attachment #9034909 - Flags: review?(mstange)
Attachment #9031710 - Attachment is obsolete: true
Attachment #9031710 - Flags: review?(mstange)
Comment on attachment 9034909 [details] [diff] [review]
Add TextMarker payloads for simple Profiler Markers

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

Sorry for the delay. This looks good.

I reconsidered my opinion on printf-style markers and I'm not opposed to them any more, at least not as much as I was before. They allow easily adding a one-off marker during debugging, they automatically format the data in the way you'd like to see them in perf.html, new markers they don't require any changes to perf.html, and having individual numbers in separate data fields is only really useful if you have some code that automatically consumes those fields, and we don't have such code at the moment.
Attachment #9034909 - Flags: review?(mstange) → review+

Actually, it seems like almost every user of TextMarker ends up calling NS_ConvertASCIItoUTF16, and then TextMarkerPayload::StreamPayload converts the text back to UTF8 using NS_ConvertUTF16toUTF8.

Could you make the constructors take const nsACString& parameters instead?

Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f053802584
Add TextMarker payloads for simple Profiler Markers r=mstange
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.