Consider removing the Stopwatch perf monitoring code

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: jandem, Assigned: tarek)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox66 fixed)

Details

(Whiteboard: [js:tech-debt])

Attachments

(1 attachment)

Reporter

Description

2 years ago
The Stopwatch constructor/destructor add overhead to RunScript.

According to Yoric it's only used for website perf in about:performance now and we can probably remove it: 

<Yoric> Not that much anymore. gijs removed it from add-ons for 57, so it still measures performance of webpages, for the sake of about:performance.
<Yoric> It should not take any visible time when about:performance is closed. If it does, that's a bug.
<Yoric> 	jandem: Anyway, since nobody actually makes real use of the Stopwatch, I guess we should just remove it if there is a cost.
<Yoric> It was meant to be useful for a feature that never landed.
Reporter

Comment 1

2 years ago
Gijs, mconley: any thoughts on removing the "Performance of Web pages" data from about:performance?
Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 2

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #1)
> Gijs, mconley: any thoughts on removing the "Performance of Web pages" data
> from about:performance?

At that point, we're basically removing about:performance as a whole, right? There's no other data there than "performance of web pages".

I don't have strong feelings about it. I've used the web side of about:performance in the past, but I expect the usecase would probably be better served by an add-on that used the gecko profiler and did some automatic analysis of which tabs/windows were taking up cycles, rather than having several competing APIs to get this kind of information with.

I'm not really sure who owns making a call about about:performance if not Yoric. I was quite... pushy... getting rid of the add-ons stuff in bug 1309946 because to me it seemed like it was basically not useful, potentially harmful/confusing, and there was a significant chunk of code involved that made a runtime impact (though there's some level of confusion/debate about exactly how large the runtime impact was), and no interest/time to work on improving the feature or making better use of the data.

That last part seems to still be the case. It would be nice if we could replace the "user-facing" side of about:performance with something based on the profiler APIs, but depending on what the costs of keeping the status quo are, it may make sense to remove it even without a replacement (other than perf.html and friends, as-is). It depends what the costs are, and I don't know the answer to that question. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Reporter

Comment 3

2 years ago
(In reply to :Gijs (not here until monday) from comment #2)
> At that point, we're basically removing about:performance as a whole, right?
> There's no other data there than "performance of web pages".

There's also the "Memory usage of Subprocesses" table in my Nightly build.

This is adding non-trivial overhead to SpiderMonkey, both in terms of performance and maintainability, so if we could remove it that would be great for us.
Priority: -- → P3
Whiteboard: [js:tech-debt]
One thing to recall: the main reason for which I had to come up with the Stopwatch, rather than reusing the profiler API, was that, after having discussed it with BenWa, we concluded that the profiler API was too heavy-handed/expensive for the task. I do not remember the exact criteria, but the profiler API wakes up the entire process once per ms (even if the process is paged out, if my memory serves), which is pretty bad, battery-wise and possibly CPU-wise. Also, I seem to remember that it uses lots of memory.

So, while I have no problem with the idea of switching from the Stopwatch to something else, please profile the profiler if that's your replacement solution.
about:performance is mostly unowned, but it's also not really talked about widely. There's no UI that leads to it, for example, except for about:about.

I, personally, still find the memory reporter in about:performance to be useful - at the very least to get a heads up on which content processes are responding (and what their PIDs are).

I'm okay removing the website slowness stuff, fwiw. If that's (ironically) slowing stuff down, then let's get rid of it.
Flags: needinfo?(mconley)
I've actually been considering using this to get telemetry about jank caused by extension content scripts. But if it has that high a performance cost, that may not be a good enough reason to keep it.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)
> I've actually been considering using this to get telemetry about jank caused
> by extension content scripts. But if it has that high a performance cost,
> that may not be a good enough reason to keep it.

That Telemetry was available until Gijs removed it :)

Comment 8

2 years ago
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #5)
> about:performance is mostly unowned, but it's also not really talked about
> widely. There's no UI that leads to it, for example, except for about:about.

about:support links to it, FWIW.
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #7)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #6)
> > I've actually been considering using this to get telemetry about jank caused
> > by extension content scripts. But if it has that high a performance cost,
> > that may not be a good enough reason to keep it.
> 
> That Telemetry was available until Gijs removed it :)

Sort of. We're looking for more targetted telemetry specifically for content scripts, at this point. We know that there are extensions that inject huge amounts of JS into every frame of every page, or use lots of expensive mutation observers or tree walkers on every page, but we don't currently have a way to quantify the effects in the wild. Or to identify problem extensions before they become big/widespread-enough problems that people file bugs.

The data that we used to get was, as I recall, fairly generic, and just told us about the time spent in any extension compartment, and tended to be way too noisy to be useful. But with out-of-process extensions, what we mainly care about at this point is the code running in content scripts, since that has a fairly predictable effect on overall browser performance and responsiveness.
(In reply to Away for a while from comment #8)
> about:support links to it, FWIW.

Well spotted.
Assignee

Comment 11

9 months ago
semi-related: I am working on bug 1482392 which is re-introducing metrics for webextensions content scripts. So maybe we can take this opportunity to remove the remaining performance code that's not useful anymore in StopWatch in thhe same patch.
Assignee

Updated

8 months ago
Assignee: nobody → tarek
Assignee

Comment 12

8 months ago
So I ended up removing everything in the patch I have done for bug 1482392 - I guess we can close this one once it lands
Assignee

Comment 13

8 months ago
Removes StopWatch code and related
Attachment #9013657 - Attachment description: Bug 1406872 - Remove perf monitoring code - r?jandem → Bug 1406872 - Remove perf monitoring code - r?jandem,mossop
Attachment #9013657 - Attachment description: Bug 1406872 - Remove perf monitoring code - r?jandem,mossop → Bug 1406872 - Remove perf monitoring code - r?jandem,florian
Assignee

Comment 14

8 months ago
We will land this patch a few days after Bug 1496506 lands
Assignee

Updated

8 months ago
Depends on: 1496506
Depends on: 1498195
(In reply to Tarek Ziadé (:tarek) from comment #14)
> We will land this patch a few days after Bug 1496506 lands

Is this meant to have landed by now? Or is it waiting for bug 1498195?

If you want Florian to review the patch as well, then you'd need to change him to a "Blocking Reviewer", as otherwise only the first person to review the patch will see it in their queue.
Flags: needinfo?(tarek)
See Also: → 1356036
Assignee

Comment 16

5 months ago

It's waiting for bug 1498195 - once it lands I'll just have to tweak the patch to discard changes done in the front end.

It's up to Florian if he wants to review it, but I consider :jandem review good enough for the back end, since the final patch won't change the front end anymore

Flags: needinfo?(tarek)

Comment 17

4 months ago

(In reply to Tarek Ziadé (:tarek) from comment #16)

It's waiting for bug 1498195 - once it lands I'll just have to tweak the patch to discard changes done in the front end.

It's up to Florian if he wants to review it, but I consider :jandem review good enough for the back end, since the final patch won't change the front end anymore

Sounds like this is ready to go now?

Flags: needinfo?(tarek)
Assignee

Comment 18

4 months ago

Yes, I will rework the patch monday and we can land it :)

Flags: needinfo?(tarek)
Attachment #9013657 - Attachment description: Bug 1406872 - Remove perf monitoring code - r?jandem,florian → Bug 1406872 - Remove perf monitoring code - r?jandem,gisj
Attachment #9013657 - Attachment description: Bug 1406872 - Remove perf monitoring code - r?jandem,gisj → Bug 1406872 - Remove perf monitoring code - r?jandem,gijs

Comment 21

4 months ago
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/789fdfc911f7
Remove perf monitoring code - r=jandem,Gijs
Duplicate of this bug: 1356036

Comment 23

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.