1152884 - Add timeline markers to trace synchronous XHR

Status: RESOLVED WONTFIX

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sync XHR is definitely something we should surface because it is so slow and pretty much guaranteed to cause jank.

Comment 1

[Victor Porof [:vporof][:vp]]

Triaging. Filter on ADRENOCORTICOTROPIC (yes).

Comment 2

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1152884 - Add Timelines to trace Synchronous XHR; r?vporof

Add Timelines to trace Synchronous XHR

Comment 3

[Thomas Wisniewski [:twisniewski]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0655c708bb5167a7c0997c675585540d719c3649

Comment 4

[Thomas Wisniewski [:twisniewski]]

Julien, would you know if there is someone I should request a review from? I'm not sure if Victor was the right choice, in hindsight.

Comment 5

[Julien Wajsberg [:julienw]]

Hey Thomas,
I added a review request for Greg, he should be able to look at it when he comes back next week.

That said we don't do much development on this tool anymore, rather we focus on the new profiler in http://profiler.firefox.com/. Is there anything missing on that profiler that makes you want use the old one? Maybe the complexity is too high?

Happy to help if needed!

Comment 6

[Thomas Wisniewski [:twisniewski]]

Thanks Julien! I can't really think of anything in this profiler that I need, but I saw an open bug that sounded useful to fix, and wanted to take a stab at fixing it :)

(btw, if we're planning on decommissioning this old profiler anytime soon, then I don't mind letting this patch go).

Comment 7

[Julien Wajsberg [:julienw]]

Maybe it could be useful to have this feature in the new profiler. Hey Nazim, do you know if we already have this? If not, maybe we can file a separate bug and you could provide some information so that somebody (maybe Thomas :) ) can pick it?

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Hey Julien, I don't see any markers for synchronous XHRs. But we have label frames for XHR start and stop requests(but doesn't check if it's synchronous or not):
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/dom/xhr/XMLHttpRequestMainThread.cpp#1750
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/dom/xhr/XMLHttpRequestMainThread.cpp#2022

Maybe it would be good to have a tracing marker for both start and stop markers. But I'm not sure if we always visit OnStopRequest whenever we visit OnStartRequest(for example we also have CloseRequest). Do you want to add it?

Comment 9

[Julien Wajsberg [:julienw]]

XHR in general can be probably found with the network markers; but synchronous XHR are certainly a big problem, and I'm not sure we have a good way to find them easily nowadays.

Comment 10

[Julien Wajsberg [:julienw]]

As a data point, here is a profile of a workload using a sync xhr: https://perfht.ml/2TWZ5Am

We see the problem quite easily in an inverted tree, but there's a few problems:

1. the WebIDL edge isn't shown in the JS view. I think we'll fix this in the coming months.
2. I see that the frame label we add is XMLHttpRequest.send for all types of XHR. Maybe we should add the type of XHR in that frame label (sync/async).
3. the network marker doesn't have the information of whether it's a XHR or a script or a style etc. We should add that as well.

Comment 11

[Thomas Wisniewski [:twisniewski]]

I also see other syncloops being used in ScriptLoader.cpp and FileReaderSync.cpp. Maybe it's worth showing markers for MainThreadStopSyncLoopRunnables in general? (If we don't already do so).

Comment 12

[Greg Tatum [:gregtatum] (REO Fx132)]

I'm looking at this briefly, as I'm still at a conference, but it would probably be better to show this as either a gecko profiler marker, or as a stack label. This is currently implemented as a docshell marker, which the devtools performance tool is only using. It will be more future proof this way.

https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/tools/profiler/public/GeckoProfiler.h#620

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D23800	Bug 1152884 - Add Timelines to trace Synchronous XHR; r?vporof	twisniewski	gregtatum: Resigned from review

:twisniewski, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Comment 14

[Julien Wajsberg [:julienw]]

I don't think we want this patch as it is currently, because it adds old-style markers. But it may be a good idea to have them in the new profiler?

Comment 15

[Thomas Wisniewski [:twisniewski]]

Yes, it might be good to have them in the modern profiler. Ditto for any other situations where we might trigger a synchronous fetch from JS (though XHR is the only place I recall where that might happen off-hand).

I'll close this bug as WONTFIX given that it was intended for the old profiler.