1688943 - Consider converting timestamps in PerformanceResourceTiming and PerformanceNavigationTiming on creation

Status: NEW

(Core :: Performance, enhancement, P3)

Description

[Markus Stange [:mstange]]

At the moment, we call ReduceTimerPrecision on every field access to responseEnd. We have a cache for the startTime field (from bug 1675543) but we don't cache the other fields.

Since these fields can be accessed many times (see 1686930 comment 21 and https://share.firefox.dev/3sYTT1B), we should not do expensive work on each field access.

I think we have multiple options:

• Cache each field individually, on first access
• Convert all fields once any field of a performance entry is accessed
• Always convert all fields on entry creation

In this bug I'm going to put up a patch for that last option, always converting all fields on entry creation.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1688943 - Convert PerformanceResourceTiming timestamps eagerly. r=smaug

Comment 2

[Markus Stange [:mstange]]

Bad news: This patch turns our tests rather orange. https://treeherder.mozilla.org/jobs?repo=try&revision=f828600263eadc8650b792f65305e956a3395bd9

I have debugged it a little bit and it seems to be caused by the fact that we're calling GetRandomTimelineSeed() in the PerformanceNavigationTiming constructor. This causes NSS to be initialized earlier than usual, via the creation of the nsIRandomGenerator service. Somehow, the earlier NSS initialization causes breakage related to certificates and related to decryption of saved logins. I'm not sure why (and it sounds brittle), but I wasn't intending on causing NSS to be initialized earlier with this patch, so I'll have to fix that anyway.

This is the stack for the creation of the first PerformanceNavigationTiming object:

mozilla::dom::PerformanceNavigationTiming::PerformanceNavigationTiming(mozilla::UniquePtr<mozilla::dom::PerformanceTimingData, mozilla::DefaultDelete<mozilla::dom::PerformanceTimingData> >&&, mozilla::dom::Performance*, nsTSubstring<char16_t> const&)
mozilla::dom::PerformanceMainThread::CreateNavigationTimingEntry()
mozilla::dom::Performance::CreateForMainThread(nsPIDOMWindowInner*, nsIPrincipal*, nsDOMNavigationTiming*, nsITimedChannel*)
nsPIDOMWindowInner::CreatePerformanceObjectIfNeeded()
nsPIDOMWindowInner::GetPerformance()
mozilla::dom::Window_Binding::get_performance(JSContext*, JS::Handle<JSObject*>, void*, JSJitGetterCallArgs)
get Window.performance
mozilla::dom::Window_Binding::ClearCachedPerformanceValue(JSContext*, nsGlobalWindowInner*)
nsGlobalWindowInner::InitDocumentDependentState(JSContext*)
nsGlobalWindowOuter::SetNewDocument(mozilla::dom::Document*, nsISupports*, bool, mozilla::dom::WindowGlobalChild*)
nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::dom::WindowGlobalChild*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool)
nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::dom::WindowGlobalChild*)
nsDocShell::SetupNewViewer(nsIContentViewer*, mozilla::dom::WindowGlobalChild*)
nsDocShell::Embed(nsIContentViewer*, mozilla::dom::WindowGlobalChild*, bool, bool)
nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIPrincipal*, nsIContentSecurityPolicy*, nsIURI*, mozilla::Maybe<nsILoadInfo::CrossOriginEmbedderPolicy> const&, bool, bool, mozilla::dom::WindowGlobalChild*)
nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIPrincipal*, nsIContentSecurityPolicy*)
nsAppShellService::JustCreateTopWindow(nsIAppWindow*, nsIURI*, unsigned int, int, int, bool, mozilla::AppWindow**)
nsAppShellService::CreateHiddenWindow()
nsAppStartup::CreateHiddenWindow()
XREMain::XRE_mainRun()
XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)
XREMain::XRE_main
XRE_main(int, char**, mozilla::BootstrapConfig const&)
main

The test I debugged was the TestFirefoxRefresh.testFxANoSync marionette test, which fails because this call to decrypt fails.

I will try to make the conversion lazy, so that we don't accidentally end up moving NSS initialization earlier.

Comment 3

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mstange, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

peterv, do you recall why performance is [StoreInSlot] and not [Cached]?
[StoreInSlot] forces creating the object rather early here.

Bug 1017425 added the annotation.

Hmm, and Performance object isn't super small anymore, given all the buffers it has, so perhaps creating it lazily would be better.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to Olli Pettay [:smaug] from comment #4)

peterv, do you recall why performance is [StoreInSlot] and not [Cached]?
[StoreInSlot] forces creating the object rather early here.

I think because using StoreInSlot wasn't a problem. We can try switching it to Cached. Bug 824495 has the performance issues, so we could double-check that it doesn't regress things there.

Comment 6

[Peter Van der Beken [:peterv]]

Note that Cached can be quite a bit slower, because it doesn't have the JIT integration that StoreInSlot has.

Comment 7

[Peter Van der Beken [:peterv]]

Actually, I misremember, both have some JIT integration but StoreInSlot is still faster.

Comment 8

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #2)

I will try to make the conversion lazy, so that we don't accidentally end up moving NSS initialization earlier.

I don't think I got around to that. I think this would be the next step to finish this bug up.

I don't know if timestamp conversion in performance APIs is still a problem.