1527213 - Find a way to not use the hidden window in PerfService.jsm

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P3)

Description

[:Felipe Gomes (needinfo for replies!)]

Bug 827976 intends to make the hidden window to be lazily created only when needed (except on Mac when it's always needed), and ideally never needed on a normal browsing session.

The problem is that the potential win of not creating it is undone shortly after the browser starts up, because Activity Stream uses it through the file PerfService.jsm.

Could we find a different solution for this timekeeping in the parent process? One complication is that this code is shared both for privileged and non-privileged code, but maybe it could be decoupled..

If there's an open activity stream tab, we can assume there's always a real browser window that could be used to get window.performance. Ideally that window would be passed in as a parameter, but there are a lot of different callsites using PerfService.

We could also just get the most recent browser window, but that would probably need to be gotten every time (through a getter) because if we just keep a reference to an specific window it might be closed.

Alternatively, there are other tools to measure time in the parent, such as TelemetryStopwatch, which offers a lot of the same functionality, but with a widely different API.

Comment 1

[Ed Lee :Mardak]

The main process users of PerfService currently seems to be limited to 3 jsms: PersonalityProvider, TelemetryFeed, TopStoriesFeed

https://searchfox.org/mozilla-central/search?q=perfservice&path=newtab%2Flib

I believe TelemetryFeed related uses are triggered by browser UI but the other two seem to be UI independent as they prepare content to be rendered in future windows/tabs.

dmose, did you look at other ways to measure timing in main process?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Another alternative is that there's a window independent Cu.now() that can be used. It already takes care of tracking a fixed time origin: https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/js/xpconnect/src/XPCComponents.cpp#2429-2434

Unfortunately Cu doesn't provide the other parts of the window.performance API, but maybe the mark and getEntriesByName can be written in plain JS.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to Ed Lee :Mardak (PTO 2/2 - Feb 10) from comment #1)

The main process users of PerfService currently seems to be limited to 3
jsms: PersonalityProvider, TelemetryFeed, TopStoriesFeed

https://searchfox.org/mozilla-central/search?q=perfservice&path=newtab%2Flib

I believe TelemetryFeed related uses are triggered by browser UI but the
other two seem to be UI independent as they prepare content to be rendered
in future windows/tabs.

Interesting. PersonalityProvider and TopStoriesFeed only use perfService for .absNow(), so I think that can be easily replaced by Cu.now(). The only harder conversion would be TopStoriesFeed

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

So the complicated piece here is that the timestamps are supposed to be comparable across the main process and the child processes, because some events start in the parent (eg clicking on the UI) and finish in the child (rendering some specific thing in the about:{home,newtab} content. The PerfService is believed to be "good enough" at that for what we need, though I'm not terribly confident about that, particularly since performanceTiming timestamps have since been throttled to help mitigate Meltdown and Spectre.

mozilla::TimeStamp might be a better choice, since the markers created with that would be against the same origin as those in the profiler, I believe (we use performance.mark and friends in part because we can get the stuff marked on the profiler timeline). Even that might be problematic, since, last time I looked at that (quite a while ago), mozilla::TimeStamps seemed to not be comparable across processes, because it had been deemed more complicated than it was worth to implement. But maybe mozilla::TimeStamp does better these days; perhaps :gregtatum or :mconley knows. Or maybe one of them knows what the best practices for generating timestamps and computing durations cross-process are these days...

TelemetryStopwatch and the window independent Cu.now both appear to wrap mozilla::TimeStamp, but TelemetryStopwatch appears to be very tightly coupled to the m-c telemetry code.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

And, to be explicit, all the timestamping we want to do is in JavaScript, some in JSM-land in the parent, and some in about:{newtab, home, etc.} running in content space (but in their own process these days, not normal child processes, I believe).

Comment 6

[Greg Tatum [:gregtatum]]

In the profiler we go through all of our data, and offset it based on the parent process timing. We do this on the front-end though, so we defer until then to have the values match.

https://github.com/devtools-html/perf.html/blob/master/src/profile-logic/process-profile.js#L872-L940

Comment 7

[Greg Tatum [:gregtatum]]

Here's where we are getting the offsets:

https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/tools/profiler/core/platform.cpp#1972-1975

Which looks like it's using TimeStamp::ProcessCreation

https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/tools/profiler/core/platform.cpp#241

I haven't dug much further than this, but no one's been complaining about our data being off... so far.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Thanks, Greg! So the story around cross-process timestamp stuff suggests that on modern hardware, you can compare them, but not necessarily on older Windows machines (which is where one would presumably want good telemetry data the most. Bug 1321878 comment 10 and later have the summary as well as various gory details and Bug 1336238 is the thing that's still open.

I'm going to take a stab and speculate that something mozilla:TimeStamp based is probably the way to go, and we end up living with data of unknown quality on old Windows machines (not sure how many). I could imagine extending Cu to include the pieces of the performance timing API we want, though I have no sense of how much work that would likely be.

I'll be interested to hear mconley's thoughts here...

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #8)

I'll be interested to hear mconley's thoughts here...

Hey, thanks for tagging me in. :)

Having just read bug 1321878 comment 10, I've come to the conclusion that I don't know enough about the OS-level implementations of TimeStamps to speak with tooooooo much authority, however, here are some random thoughts:

1. As we continue to work on performance, and as we continue to create new process types, I think we all agree that it's necessary to have the ability to measure a delta between timestamps captured in different processes, and be able to trust that delta.

2. If it's not physically possible to get an accurate delta between the timestamps of two processes on certain types of hardware, we need to poison our measurements to make that inability clear. I guess that's what bug 1336238 kinda resembles.

3. Reading the rest of bug 1321878, it sounds like it's unclear whether or not the "low resolution" mode for timestamps on low-end hardware are consistent. That sounds like a blank spot in our understanding that we need to clear up. Maybe we can get Honza's help here.

If it turns out via (3) that the low-resolution timestamps are in fact consistent, then (2) can maybe be modified so that instead of saying that the delta is unreliable, we say that the delta has wider error bars.

4. Ideally, we need to be able to get these timestamps in a way that's easy for privileged JS to get at in all processes, with or without a DOM window.

It seems like we already get some of (4) via Cu.now(), which uses mozilla::TimeStamp, and returns a delta between that process creation and the current time.

If it turns out from (3) that we can trust mozilla::TimeStamp to be consistent, then perhaps what we need is to have Cu.now() return mozilla::TimeStamp::Now(), and have something like Cu.processCreation() return the process creation time. If mozilla::TimeStamp is consistent in the low-res case, then mozilla::TimeStamp::Now() from one process delta'd with a mozilla::TimeStamp::Now() from another would give us what we need... no?

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #9)

4. Ideally, we need to be able to get these timestamps in a way that's easy for privileged JS to get at in all processes, with or without a DOM window.

I think it depends on what you mean by "privileged". Last time I checked, I think about:newtab and about:home were loading in a content window running with a null-principal and getting special data from chrome via the message port.

Comment 11

[Tim Spurway [:tspurway]]

dan, could you route this one

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

So I think that there are really two pieces here:

A) create something timestampy that is cross-process consistent and usable from all required JS contexts, possibly just by extending Cu.now (effectively most of what :mconley said in comment 9)
B) change the AS PerfService code to use that thing

I propose that we should spin out A into its own bug, which lives somewhere other than the New Tab component, and leave this bug to represent B.

:mconley, let me know if that sounds reasonable to you, and if it does, I can do the bug cloning into whatever component you'd suggest...

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #10)

I think it depends on what you mean by "privileged". Last time I checked, I think about:newtab and about:home were loading in a content window running with a null-principal and getting special data from chrome via the message port.

Yes, that's true - but there are still privileged portions of it that run in the content process (AboutNewTabService comes to mind).

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #12)

:mconley, let me know if that sounds reasonable to you, and if it does, I can do the bug cloning into whatever component you'd suggest...

Tentatively, this sounds reasonable, but I think it might be better if Honza or someone more familiar with the underlying complexities of cross-process timers would weigh in.

Hey mayhemer, do you have any thoughts to add here?

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #13)

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #10)

I think it depends on what you mean by "privileged". Last time I checked, I think about:newtab and about:home were loading in a content window running with a null-principal and getting special data from chrome via the message port.

Yes, that's true - but there are still privileged portions of it that run in the content process (AboutNewTabService comes to mind).

Right. What I was trying to be explicit about is that writing something that only works in privileged code wouldn't cover all the use cases.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #13)

I think it might be better if Honza or someone more familiar with the underlying complexities of cross-process timers would weigh in.

Hey mayhemer, do you have any thoughts to add here?

Hmm.. just let me make sure I understand: you want a function that will return an (opaque) absolute monotonic timestamp that can be compared cross-process? AFAICT, on modern Windows (8+) versions and (relatively) modern hardware TimeStamp::Now() will be equal cross-process; there is bug 1515155 to fix, but it's a minor thing. I'm not sure how clock_gettime() on posix systems and mach_absolute_time() on osx behave, but I believe it will be very similar.

We are sending TimeStamps (not TimeDurations!) through IPC interfaces on daily bases, so I think this has already been established to be usable cross process well.

I think Markus may know more, Gecko Profiler is definitely dependent on TimeStamp::Now, cross-process.

This is very hard to detect if faulty, though.

Comment 16

[Markus Stange [:mstange]]

I agree with Honza - I know of graphics code and events code which assumes that mozilla::TimeStamps can already be meaningfully passed and compared between processes. I never looked into whether that's actually the case in all configurations... I'm hoping that, if comparing TimeStamps from different processes wasn't meaningful, it would already have shown up as a problem elsewhere.
On macOS, mach_absolute_time() is certainly comparable between processes.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

This blocks the xpcshell test I wrote in bug 1622088 because it seems that we sometimes call into the PerfService in that test, which attempts to access the hidden window, which doesn't exist for xpcshell tests.

So I guess I'll take this.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1527213 - Stop using PerfService in DiscoveryStreamFeed, RecommendationProviderSwitcher and TopStoriesFeed. r?k88hudson!

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1527213 - Stop using PerfService from TelemetryFeed. r?k88hudson!

Depends on D72737

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1527213 - Remove privileged-script support and hidden window usage from PerfService.jsm. r?k88hudson!

Depends on D72738

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1527213 - Only ship PerfService inside of activity-stream.bundle.js. r?k88hudson!

Depends on D72740

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f930bc5fa581
Stop using PerfService in DiscoveryStreamFeed, RecommendationProviderSwitcher and TopStoriesFeed. r=k88hudson
https://hg.mozilla.org/integration/autoland/rev/f24658cef948
Stop using PerfService from TelemetryFeed. r=k88hudson
https://hg.mozilla.org/integration/autoland/rev/5433013a8dbd
Remove privileged-script support and hidden window usage from PerfService.jsm. r=k88hudson
https://hg.mozilla.org/integration/autoland/rev/92f454a990a6
Only ship PerfService inside of activity-stream.bundle.js. r=k88hudson

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f930bc5fa581
https://hg.mozilla.org/mozilla-central/rev/f24658cef948
https://hg.mozilla.org/mozilla-central/rev/5433013a8dbd
https://hg.mozilla.org/mozilla-central/rev/92f454a990a6