Closed Bug 1415545 Opened 2 years ago Closed 2 years ago

Document working on performance

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

Document typical slownesses:
* Too many react renders
* Slow CSS rules like flexbox
* Waiting for non-important async operations before doing what user really expect
* Updating invisible elements
* Load too many modules
* Too many reflows
* Too many redux actions

Also document how to efficiently work on perf:
* Use perf-html
* Use in-code timers with performance.now()
* Use markers and perf-html (performance.mark/measure)
* Use screenshot/canvas snippet
* Quick fix / early return / commenting code
* Push to damp
Summary: Document working on performances → Document working on performance
Priority: -- → P3
Comment on attachment 8936773 [details]
Bug 1415545 - Land images related to performance documentation page in DevTools.

https://reviewboard.mozilla.org/r/207484/#review213618

Hey - I added a ton of ideas and suggestions, but this is a good start. Feel free to merge it and keep working on it and ask someone else for a review. I'll trust you and we can always make more refinements to this doc. Merci beaucoup!

::: devtools/docs/SUMMARY.md:17
(Diff revision 1)
>        * [ESLint](./contributing/eslint.md)
>      * [CSS](./contributing/css.md)
>    * [Creating and sending patches](./contributing/making-prs.md)
>    * [Code reviews](./contributing/code-reviews.md)
>    * [Filing good bugs](./contributing/filing-good-bugs.md)
> +  * [Working on performance](./contributing/performance.md)

Didn't we mention it'd be better to use something like 'Writing efficient code'?

::: devtools/docs/contributing/performance.md:1
(Diff revision 1)
> +# Working of performance

s/of/on

Although this should reflect the final document title anyway...

::: devtools/docs/contributing/performance.md:3
(Diff revision 1)
> +# Working of performance
> +
> +To be efficient while working on performance, you should define important user scenarios: a typical set of actions a user would do.

You're missing the 'why'. Why would a developer want to 'work on performance'?

There should be something explaining something like code in devtools can be slow, and that it is a source  of frustration for our users, and we don't want that. Also when using firefox in debugging mode, then it is even slower! so the faster we are, the better. And this section aims to help developers identify slowness in the code and fix it.

Feel free to add any other ideas I might have missed-this is just a starting point!

::: devtools/docs/contributing/performance.md:5
(Diff revision 1)
> +# Working of performance
> +
> +To be efficient while working on performance, you should define important user scenarios: a typical set of actions a user would do.
> +
> +A couple of examples in DevTools:

I don't quite see how a devtools developer would think of "oh yeah, let me define a user scenario to make it fast". Were you thinking more on terms of "in DevTools, we aim to make common interactions fast, for example... this list"?

::: devtools/docs/contributing/performance.md:15
(Diff revision 1)
> +* Call console.log() 1000 times with complex arguments with the tools off
> +* Call console.log() 1000 times with complex arguments while the console is opened
> +* With the inspector opened, select another element in the markup view and see the rule view being updated
> +* ...
> +
> +But it could also be less typical. For example when you receive a bug report, where someone mention a precise environment making an uncommon scenario be slow.

I'm not sure what to do about this sentence.

I don't know if you mean that people have to 'define an scenario' when they get a slowness bug report.

Or if you mean that apart from using the knowledge in this page to make common interactions fast, you can use it to make other sorts of interactions fast, even if they're edge cases/uncommon scenarios.

If it's the latter, perhaps you could rephrase this sentence as "The techniques described in this page should help you to make code fast and efficient both when working in common interactions and in edge cases; for example, when someone reports slowness under very specific circumstances". How does it sound?

::: devtools/docs/contributing/performance.md:16
(Diff revision 1)
> +* Call console.log() 1000 times with complex arguments while the console is opened
> +* With the inspector opened, select another element in the markup view and see the rule view being updated
> +* ...
> +
> +But it could also be less typical. For example when you receive a bug report, where someone mention a precise environment making an uncommon scenario be slow.
> +Then pick one scenario you want to make faster and focus on just this one.

I've absolutely no idea what you mean with 'just focus on one scenario'. Do you mean that when trying to improve performance results, developers should try to focus on fixing one result at a time? If so, it's hard to understand from this sentence.

::: devtools/docs/contributing/performance.md:20
(Diff revision 1)
> +But it could also be less typical. For example when you receive a bug report, where someone mention a precise environment making an uncommon scenario be slow.
> +Then pick one scenario you want to make faster and focus on just this one.
> +
> +## Don't guess — profile.
> +
> +The very first thing to do is to record a profile while reproducing the scenario.

I'm not quite convinced about 'reproducing the scenario', I'd go for something more like 'repeating the sequence of steps that demonstrate slowness'.

Also, it'd be relevant to say that it's important to have steps to reproduce, that actually reproduce, with as much data as possible.

::: devtools/docs/contributing/performance.md:22
(Diff revision 1)
> +
> +## Don't guess — profile.
> +
> +The very first thing to do is to record a profile while reproducing the scenario.
> +
> +Please look into this [first documentation](https://developer.mozilla.org/docs/Mozilla/Performance/Reporting_a_Performance_Problem)

You can shorten this as

"Here's the Firefox documentation for [how to install the profiler and record a profile](link) and also [how to interpret the profiles](link)".

::: devtools/docs/contributing/performance.md:27
(Diff revision 1)
> +Please look into this [first documentation](https://developer.mozilla.org/docs/Mozilla/Performance/Reporting_a_Performance_Problem)
> +on how to install the profiler and record a profile.
> +And then have a look at this [other documentation](https://developer.mozilla.org/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler#Understanding_Profiles)
> +describing how to interpret the profiles.
> +
> +Here is some specifics about DevTools architecture that are worth knowing when looking at a profile:

s/Here is/There are
s/specifics about/peculiarities in the 
s/knowing/knowing about

::: devtools/docs/contributing/performance.md:28
(Diff revision 1)
> +on how to install the profiler and record a profile.
> +And then have a look at this [other documentation](https://developer.mozilla.org/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler#Understanding_Profiles)
> +describing how to interpret the profiles.
> +
> +Here is some specifics about DevTools architecture that are worth knowing when looking at a profile:
> +* Tweak profiler default settings.

As usual, you use bullet points instead of subsections :) Please use ### subsections instead of bullet points, and fix the indentation!

::: devtools/docs/contributing/performance.md:30
(Diff revision 1)
> +describing how to interpret the profiles.
> +
> +Here is some specifics about DevTools architecture that are worth knowing when looking at a profile:
> +* Tweak profiler default settings.
> +
> +  The default buffer size is too small. If you don't increase it, you may easily miss data and only see last couple of seconds of your record.

recordING

::: devtools/docs/contributing/performance.md:31
(Diff revision 1)
> +
> +Here is some specifics about DevTools architecture that are worth knowing when looking at a profile:
> +* Tweak profiler default settings.
> +
> +  The default buffer size is too small. If you don't increase it, you may easily miss data and only see last couple of seconds of your record.
> +  To increase the buffer size, click on profiler add-on icon, in Firefox toolbar, and set it to 360MB like this:

click on THE profiler
in THE Firefox toolbar

s/360MB/360MB, /

::: devtools/docs/contributing/performance.md:33
(Diff revision 1)
> +* Tweak profiler default settings.
> +
> +  The default buffer size is too small. If you don't increase it, you may easily miss data and only see last couple of seconds of your record.
> +  To increase the buffer size, click on profiler add-on icon, in Firefox toolbar, and set it to 360MB like this:
> +
> +    <img src="performance/profiler-buffer-size.png" alt="Profiler buffer size" style="width: 300px" />

You should use the Markdown syntax for images (also why are you specifying the image size? the documentation should adapt automatically to max-width)

::: devtools/docs/contributing/performance.md:35
(Diff revision 1)
> +  The default buffer size is too small. If you don't increase it, you may easily miss data and only see last couple of seconds of your record.
> +  To increase the buffer size, click on profiler add-on icon, in Firefox toolbar, and set it to 360MB like this:
> +
> +    <img src="performance/profiler-buffer-size.png" alt="Profiler buffer size" style="width: 300px" />
> +
> +  The only other useful setting to play with when debugging DevTools is the `Interval`.

The only other useful... <--- this sounds so dismissive!!!

Use something less negative: The other setting worth mentioning for DevTools debugging is the `Interval`

::: devtools/docs/contributing/performance.md:38
(Diff revision 1)
> +    <img src="performance/profiler-buffer-size.png" alt="Profiler buffer size" style="width: 300px" />
> +
> +  The only other useful setting to play with when debugging DevTools is the `Interval`.
> +  The profiler records only samples, based on this `Interval`.
> +  If you want to see more fine-grained stack traces, you may reduce this interval to 0.1ms.
> +  But do that only if you really need it, as it will make Firefox much more slower when recording,

`But` should go on the previous line, separated with a comma and not a full stop, as the sentence is related to the previous one.

s/more slower/slower/

Or if you want to be more specific: /even slower/

::: devtools/docs/contributing/performance.md:39
(Diff revision 1)
> +
> +  The only other useful setting to play with when debugging DevTools is the `Interval`.
> +  The profiler records only samples, based on this `Interval`.
> +  If you want to see more fine-grained stack traces, you may reduce this interval to 0.1ms.
> +  But do that only if you really need it, as it will make Firefox much more slower when recording,
> +  and the measured times will be even slower.

Which times do you mean here? Do you mean that the profile results will show even slower times than if you hadn't modified this setting?

::: devtools/docs/contributing/performance.md:41
(Diff revision 1)
> +  The profiler records only samples, based on this `Interval`.
> +  If you want to see more fine-grained stack traces, you may reduce this interval to 0.1ms.
> +  But do that only if you really need it, as it will make Firefox much more slower when recording,
> +  and the measured times will be even slower.
> +
> +* DevTools UI lives in parent process.

"The DevTools UI runs on the parent process" sounds better, I don't think any of our code is alive (I hope!)

::: devtools/docs/contributing/performance.md:43
(Diff revision 1)
> +  But do that only if you really need it, as it will make Firefox much more slower when recording,
> +  and the measured times will be even slower.
> +
> +* DevTools UI lives in parent process.
> +
> +  So when you are debugging any tool frontend, always ensure selecting `Main thread` line.

Would sound better as "When you are debugging tool front-ends (e.g. panels), always ensure you select the `Main Thread` line."

Also, is it a 'line'? or a track? It would be worth checking with Greg or Julien which term do they use.

::: devtools/docs/contributing/performance.md:51
(Diff revision 1)
> +    <img src="performance/profiler-main-thread.png" alt="Select main process" style="width: 300px" />
> +
> +  Otherwise, the vast majority of DevTools backend (DebuggerServer, actors, ...) lives in content processes.
> +  So if you are debugging them, you should select one of the `Content` lines.
> +
> +* Most of DevTools codebase is in Javascript.

Most of THE DevTools codebase

::: devtools/docs/contributing/performance.md:63
(Diff revision 1)
> +* Handy filter strings for DevTools:
> +  * `require`
> +  Helps highlighting the cost of module loading
> +    ![modules](performance/profiler-filter-require.png)
> +  * DevTools uses two kind of URLs:
> +    * `chrome://devtools/` for all panel documents. So filter with this to see the cost of all panel documents:

Remove the 'so'

::: devtools/docs/contributing/performance.md:65
(Diff revision 1)
> +  Helps highlighting the cost of module loading
> +    ![modules](performance/profiler-filter-require.png)
> +  * DevTools uses two kind of URLs:
> +    * `chrome://devtools/` for all panel documents. So filter with this to see the cost of all panel documents:
> +      ![panels documents](performance/profiler-chrome-url.png)
> +    * `resource://devtools/` for all javascript modules. So filter with this to see the cost of all modules:

Remove the 'so'

::: devtools/docs/contributing/performance.md:72
(Diff revision 1)
> +
> +// TODO: invert call tree?
> +
> +### Record durations manually
> +
> +Sometimes it is handy to focus on a very precise piece of code and record its time manually.

It'd be helpful to give some advice as to when this should be done rather than using the profiler. For example is it when you're very sure you've identified where things are slow and you want to record durations to compare while you try different variations until you make it fast?

::: devtools/docs/contributing/performance.md:74
(Diff revision 1)
> +
> +### Record durations manually
> +
> +Sometimes it is handy to focus on a very precise piece of code and record its time manually.
> +
> +#### Print durations in your Terminal

"your terminal"? This makes it sound like it's going to output to Bash.

I reckon it'd be better to say "to the console"

::: devtools/docs/contributing/performance.md:76
(Diff revision 1)
> +
> +Sometimes it is handy to focus on a very precise piece of code and record its time manually.
> +
> +#### Print durations in your Terminal
> +
> +You can use [`Performance`](https://developer.mozilla.org/docs/Web/API/Performance) API, like this:

THE Performance API

::: devtools/docs/contributing/performance.md:88
(Diff revision 1)
> +console.log("my function took", window.performance.now() - start, "ms");
> +```
> +
> +#### Use markers
> +
> +Performance API also allows recording markers, like this:

THE Performance API

::: devtools/docs/contributing/performance.md:101
(Diff revision 1)
> +```
> +
> +This marker will appear in the `Marker Chart` section in perf-html, in the `UserTiming` lines:
> +  ![custom markers](performance/profiler-custom-markers.png)
> +
> +You can double click on it to make perf-html display the record during this precice moment in time.

s/precice/precise/

::: devtools/docs/contributing/performance.md:102
(Diff revision 1)
> +
> +This marker will appear in the `Marker Chart` section in perf-html, in the `UserTiming` lines:
> +  ![custom markers](performance/profiler-custom-markers.png)
> +
> +You can double click on it to make perf-html display the record during this precice moment in time.
> +So that the call tree will only display what executed during this measurement.

s/in time.\nSo that/in time, and the call tree will only display what was executed during this ...

::: devtools/docs/contributing/performance.md:104
(Diff revision 1)
> +  ![custom markers](performance/profiler-custom-markers.png)
> +
> +You can double click on it to make perf-html display the record during this precice moment in time.
> +So that the call tree will only display what executed during this measurement.
> +
> +### Quickly prototype

Prototype quickly

::: devtools/docs/contributing/performance.md:106
(Diff revision 1)
> +You can double click on it to make perf-html display the record during this precice moment in time.
> +So that the call tree will only display what executed during this measurement.
> +
> +### Quickly prototype
> +
> +To figure out what is slow, do not hesitate to comment or disable pieces of code.

I think it'd sound better as:

Sometimes the best way to find what is slow is to comment blocks of code out and uncomment them one by one until you identify the culprit. And then focus on it.

There are few things worse than spending a long time refactoring the piece of code that was not slow to begin with!

::: devtools/docs/contributing/performance.md:113
(Diff revision 1)
> +
> +The worse would be to spend a long time refactoring something that wasn't the main slowness.
> +
> +## Assess your improvement.
> +
> +Once you have a patch that you think fixes the performance, you have to assess it actually improves it.

I think rather than 'fixing' the performance, you improve it, because the performance is a reading, it cannot be 'fixed'. It's just values...

::: devtools/docs/contributing/performance.md:113
(Diff revision 1)
> +
> +The worse would be to spend a long time refactoring something that wasn't the main slowness.
> +
> +## Assess your improvement.
> +
> +Once you have a patch that you think fixes the performance, you have to assess it actually improves it.

s/it actually/whether it actually/

::: devtools/docs/contributing/performance.md:133
(Diff revision 1)
> +  ![netmonitor with patch](performance/profiler-netmon-open-fixed.png)
> +
> +It highlights that:
> + * we no longer load StatisticsPanel,
> + * App is faster to load.
> +

That's an excellent example!

::: devtools/docs/contributing/performance.md:136
(Diff revision 1)
> + * we no longer load StatisticsPanel,
> + * App is faster to load.
> +
> +### Run performance tests
> +
> +See if any subtest reports a win. Ensure that the win makes any sense.

"win" is too jargon. Did we win the lottery? :)
Replace with 'improvement'

::: devtools/docs/contributing/performance.md:137
(Diff revision 1)
> + * App is faster to load.
> +
> +### Run performance tests
> +
> +See if any subtest reports a win. Ensure that the win makes any sense.
> +For example, if the test is 50% faster, may be you ended up breaking the performance test itself.

may be you ended up breaking the performance test itself/maybe you broke the performance test

::: devtools/docs/contributing/performance.md:138
(Diff revision 1)
> +
> +### Run performance tests
> +
> +See if any subtest reports a win. Ensure that the win makes any sense.
> +For example, if the test is 50% faster, may be you ended up breaking the performance test itself.
> +The test may no longer wait correctly for the full scenario completion.

I think it's missing some connection:

"This might happen if the test no longer waits for all the operations to finish executing before completing"

::: devtools/docs/contributing/performance.md:146
(Diff revision 1)
> +```
> +./mach try -b o -p linux64 -u none -t g2-e10s --rebuild-talos 5 --artifact
> +```
> +It will print in your Terminal a link to perfherder like this one:
> +[https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db](https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db)
> +Open it 30 minutes up to 2 hours later to see your results.

would be worth to add a note that says 
(running performance tests takes time!)

::: devtools/docs/contributing/performance.md:149
(Diff revision 1)
> +It will print in your Terminal a link to perfherder like this one:
> +[https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db](https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db)
> +Open it 30 minutes up to 2 hours later to see your results.
> +See [Performance tests (DAMP)](../tests/performance-tests.md) for more information about PerfHerder/try.
> +
> +You should aim to have DAMP reports you wins [like this](https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800):

I'd replace:

"You should aim to have DAMP reports you wins"

with

"Let's look at how to interpret an actual real-life [set of perfherder results](link to perfherder)"

::: devtools/docs/contributing/performance.md:154
(Diff revision 1)
> +You should aim to have DAMP reports you wins [like this](https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800):
> +
> +![perfherder results](performance/perfherder-results.png)
> +
> +These results are related to [lazy loading of modules in netmonitor](https://bugzilla.mozilla.org/show_bug.cgi?id=1420289).
> +It is interesting to see that this patch is a tradeoff. It makes netmonitor opening significantly faster, by preventing loading many modules during its opening.

trade-off

::: devtools/docs/contributing/performance.md:164
(Diff revision 1)
> +
> +// TODO: this is just a sketch
> +
> +### React and Redux
> +* Too many react components updates
> +* Too many redux actions

I can add some ideas! Might be worth mentioning the option of grouping similar consecutive actions and making just one update as a solution

::: devtools/docs/contributing/performance.md:174
(Diff revision 1)
> +* Too many reflows
> +
> +### Anything else
> +* Fetching/computing data that are not immediately or conditionally used
> +* Updating invisible elements
> +* Waiting for non-important async operations before doing what user really expect

Also not showing anything (just a blank rectangle) instead of an skeleton or *something*, even some feedback that something is happening/loading - so it makes the user think the code is hung or broken

::: devtools/docs/contributing/performance.md:175
(Diff revision 1)
> +
> +### Anything else
> +* Fetching/computing data that are not immediately or conditionally used
> +* Updating invisible elements
> +* Waiting for non-important async operations before doing what user really expect
> +* Load too many modules

is this related to how the Firefox loader works, or do you mean loading too much code in general? would it be better to package in a bundle? or are you worried about total file size to be processed? it seemed like in some cases disk transfer speed *is* an issue (i.e. when not using a SSD)
Attachment #8936773 - Flags: review?(spenades) → review+
(In reply to Soledad Penades [:sole] [:spenades] from comment #2)
> ::: devtools/docs/SUMMARY.md:17
> (Diff revision 1)
> >        * [ESLint](./contributing/eslint.md)
> >      * [CSS](./contributing/css.md)
> >    * [Creating and sending patches](./contributing/making-prs.md)
> >    * [Code reviews](./contributing/code-reviews.md)
> >    * [Filing good bugs](./contributing/filing-good-bugs.md)
> > +  * [Working on performance](./contributing/performance.md)
> 
> Didn't we mention it'd be better to use something like 'Writing efficient
> code'?

Yes, but I was unsure it was still reflecting the overall content of this page.
Anyway, I switched to "Writing efficient code" as I don't have strong opinion on the title.

> ::: devtools/docs/contributing/performance.md:3
> (Diff revision 1)
> > +# Working of performance
> > +
> > +To be efficient while working on performance, you should define important user scenarios: a typical set of actions a user would do.
> 
> You're missing the 'why'. Why would a developer want to 'work on
> performance'?

Ok, added a new paragraph about the why.

> ::: devtools/docs/contributing/performance.md:5
> (Diff revision 1)
> > +# Working of performance
> > +
> > +To be efficient while working on performance, you should define important user scenarios: a typical set of actions a user would do.
> > +
> > +A couple of examples in DevTools:
> 
> I don't quite see how a devtools developer would think of "oh yeah, let me
> define a user scenario to make it fast". Were you thinking more on terms of
> "in DevTools, we aim to make common interactions fast, for example... this
> list"?

The introduction was quite confusing. I removed the examples. I'm not sure they were any useful.
I completely rephrased all that.

> ::: devtools/docs/contributing/performance.md:20
> (Diff revision 1)
> > +But it could also be less typical. For example when you receive a bug report, where someone mention a precise environment making an uncommon scenario be slow.
> > +Then pick one scenario you want to make faster and focus on just this one.
> > +
> > +## Don't guess — profile.
> > +
> > +The very first thing to do is to record a profile while reproducing the scenario.
> 
> I'm not quite convinced about 'reproducing the scenario', I'd go for
> something more like 'repeating the sequence of steps that demonstrate
> slowness'.
> 
> Also, it'd be relevant to say that it's important to have steps to
> reproduce, that actually reproduce, with as much data as possible.

Steps to reproduce is something I associate with bugs. You may be working on top interactions and the term "reproducing" doesn't really work well with that.
I kept this sentence as-is. But I hope that the new introduction makes this much clearer. I do mention steps to reproduce there.

> ::: devtools/docs/contributing/performance.md:33
> (Diff revision 1)
> > +* Tweak profiler default settings.
> > +
> > +  The default buffer size is too small. If you don't increase it, you may easily miss data and only see last couple of seconds of your record.
> > +  To increase the buffer size, click on profiler add-on icon, in Firefox toolbar, and set it to 360MB like this:
> > +
> > +    <img src="performance/profiler-buffer-size.png" alt="Profiler buffer size" style="width: 300px" />
> 
> You should use the Markdown syntax for images (also why are you specifying
> the image size? the documentation should adapt automatically to max-width)

I have to. Otherwise it renders really badly in HTML.
The images are way to big to be able to read the doc correctly when using markdown.

> ::: devtools/docs/contributing/performance.md:39
> (Diff revision 1)
> > +
> > +  The only other useful setting to play with when debugging DevTools is the `Interval`.
> > +  The profiler records only samples, based on this `Interval`.
> > +  If you want to see more fine-grained stack traces, you may reduce this interval to 0.1ms.
> > +  But do that only if you really need it, as it will make Firefox much more slower when recording,
> > +  and the measured times will be even slower.
> 
> Which times do you mean here? Do you mean that the profile results will show
> even slower times than if you hadn't modified this setting?

Yes. Everything is going to be slower. The durations recorded by the profiler are going to be longer.
Your profile is going to be longer (it will take more time to replay your scenario).
And so the "running times" and "self" columns are going to show bigger durations.

> ::: devtools/docs/contributing/performance.md:43
> (Diff revision 1)
> > +  But do that only if you really need it, as it will make Firefox much more slower when recording,
> > +  and the measured times will be even slower.
> > +
> > +* DevTools UI lives in parent process.
> > +
> > +  So when you are debugging any tool frontend, always ensure selecting `Main thread` line.
> 
> Would sound better as "When you are debugging tool front-ends (e.g. panels),
> always ensure you select the `Main Thread` line."
> 
> Also, is it a 'line'? or a track? It would be worth checking with Greg or
> Julien which term do they use.

If I refer to MDN doc, the timeline is made of "rows".
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler#Understanding_Profiles
So lines looks correct here.

> ::: devtools/docs/contributing/performance.md:72
> (Diff revision 1)
> > +
> > +// TODO: invert call tree?
> > +
> > +### Record durations manually
> > +
> > +Sometimes it is handy to focus on a very precise piece of code and record its time manually.
> 
> It'd be helpful to give some advice as to when this should be done rather
> than using the profiler. For example is it when you're very sure you've
> identified where things are slow and you want to record durations to compare
> while you try different variations until you make it fast?

Added an example but not quite this exact one.

> ::: devtools/docs/contributing/performance.md:74
> (Diff revision 1)
> > +
> > +### Record durations manually
> > +
> > +Sometimes it is handy to focus on a very precise piece of code and record its time manually.
> > +
> > +#### Print durations in your Terminal
> 
> "your terminal"? This makes it sound like it's going to output to Bash.

Actually, that's what I'm using in most cases...
I'll mention both.

> ::: devtools/docs/contributing/performance.md:164
> (Diff revision 1)
> > +
> > +// TODO: this is just a sketch
> > +
> > +### React and Redux
> > +* Too many react components updates
> > +* Too many redux actions
> 
> I can add some ideas! Might be worth mentioning the option of grouping
> similar consecutive actions and making just one update as a solution

I'll drop this section for now. I can't list all this without additional explanation for each item.

> ::: devtools/docs/contributing/performance.md:175
> (Diff revision 1)
> > +
> > +### Anything else
> > +* Fetching/computing data that are not immediately or conditionally used
> > +* Updating invisible elements
> > +* Waiting for non-important async operations before doing what user really expect
> > +* Load too many modules
> 
> is this related to how the Firefox loader works, or do you mean loading too
> much code in general? would it be better to package in a bundle? or are you
> worried about total file size to be processed? it seemed like in some cases
> disk transfer speed *is* an issue (i.e. when not using a SSD)

Loading too much code in general. Bundling doesn't change anything.
Loading any byte of javascript requires significant operations to be done, even if it is cached.
Assignee: nobody → poirot.alex
Blocks: 1425907
Flags: needinfo?(jdescottes)
Comment on attachment 8936773 [details]
Bug 1415545 - Land images related to performance documentation page in DevTools.

https://reviewboard.mozilla.org/r/207484/#review215370

::: devtools/docs/SUMMARY.md:17
(Diff revision 2)
>        * [ESLint](./contributing/eslint.md)
>      * [CSS](./contributing/css.md)
>    * [Creating and sending patches](./contributing/making-prs.md)
>    * [Code reviews](./contributing/code-reviews.md)
>    * [Filing good bugs](./contributing/filing-good-bugs.md)
> +  * [Writing efficient code](./contributing/performance.md)

This is more about "Investigating DevTools performance issues" than writing efficient code. With such a title I expect guidelines and patterns to follow to avoid performance issues when writing devtools code.

::: devtools/docs/contributing/performance.md:26
(Diff revision 2)
> +
> +There are some peculiarities about DevTools architecture that are worth knowing about when looking at a profile:
> +
> +### Tweak profiler default settings
> +
> +The default buffer size is too small. If you don't increase it, you may easily miss data and only see last couple of seconds of your recording.

Mention the current buffer size (9MB) in case this becomes irrelevant in the future.

::: devtools/docs/contributing/performance.md:46
(Diff revision 2)
> +
> +  <img src="performance/profiler-main-thread.png" alt="Select main process" style="width: 300px" />
> +
> +Otherwise, the vast majority of DevTools backend (DebuggerServer, actors, ...) lives in content processes.
> +So if you are debugging them, you should select one of the `Content` lines.
> +

One thing related to Content vs Main that helped me: when investigating complex scenarios (eg. tool opening, page reload) before you could isolate the specific part of the code that is slow, it is better to investigate step by step, with smallish time ranges, rather than hoping for a hotspot to standout from the whole profile.

Something like: 
- select a range with Main process activity
  - is it DevTools? is it too slow?
- go to content processes, look for activity that has been triggered by DevTools main process
  - is it from DevTools? is it too slow?
- go back to the main process, what happened when the actor on the content process was done

::: devtools/docs/contributing/performance.md:64
(Diff revision 2)
> +  * DevTools uses two kind of URLs:
> +    * `chrome://devtools/` for all panel documents. Filter with this to see the cost of all panel documents:
> +      ![panels documents](performance/profiler-chrome-url.png)
> +    * `resource://devtools/` for all javascript modules. Filter with this to see the cost of all modules:
> +      ![modules](performance/profiler-resource-url.png)
> +

I could see an additional section that quickly explains the toolbox/panel lifecycle. 

Some tools can have side-effects on performance if they are initialized. For instance, the netmonitor and inspector will have a significant impact on page reload if they have been initialized.  

It should explain that tools are only initialized when being opened, so when investigating a scenario related to a tool make sure to only have this one initialized. One way of doing this is : select the tested tool > close toolbox > open toolbox with the default shortcut F12 (which will open on the last selected tool). Or simply close toolbox > open toolbox with a tool specific command.

Just a suggestion, I can add it after you land the first iteration.
Flags: needinfo?(jdescottes)
Blocks: 1428761
Blocks: 1428762
(In reply to Julian Descottes [:jdescottes][:julian] from comment #5)
> This is more about "Investigating DevTools performance issues" than writing
> efficient code. With such a title I expect guidelines and patterns to follow
> to avoid performance issues when writing devtools code.

I went for "Investigating performance issues".

> ::: devtools/docs/contributing/performance.md:46
> (Diff revision 2)
> > +
> > +  <img src="performance/profiler-main-thread.png" alt="Select main process" style="width: 300px" />
> > +
> > +Otherwise, the vast majority of DevTools backend (DebuggerServer, actors, ...) lives in content processes.
> > +So if you are debugging them, you should select one of the `Content` lines.
> > +
> 
> One thing related to Content vs Main that helped me: when investigating
> complex scenarios (eg. tool opening, page reload) before you could isolate
> the specific part of the code that is slow, it is better to investigate step
> by step, with smallish time ranges, rather than hoping for a hotspot to
> standout from the whole profile.
> 
> Something like: 
> - select a range with Main process activity
>   - is it DevTools? is it too slow?
> - go to content processes, look for activity that has been triggered by
> DevTools main process
>   - is it from DevTools? is it too slow?
> - go back to the main process, what happened when the actor on the content
> process was done

I filled bug 1428761 to continue the discussion there.

> ::: devtools/docs/contributing/performance.md:64
> (Diff revision 2)
> > +  * DevTools uses two kind of URLs:
> > +    * `chrome://devtools/` for all panel documents. Filter with this to see the cost of all panel documents:
> > +      ![panels documents](performance/profiler-chrome-url.png)
> > +    * `resource://devtools/` for all javascript modules. Filter with this to see the cost of all modules:
> > +      ![modules](performance/profiler-resource-url.png)
> > +
> 
> I could see an additional section that quickly explains the toolbox/panel
> lifecycle. 
> 
> Some tools can have side-effects on performance if they are initialized. For
> instance, the netmonitor and inspector will have a significant impact on
> page reload if they have been initialized.

Sounds like a good addition, I filled bug 1428762.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90b364581f60
Document working on DevTools performance. r=sole
https://hg.mozilla.org/mozilla-central/rev/90b364581f60
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
I forgot to land the images...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8936773 [details]
Bug 1415545 - Land images related to performance documentation page in DevTools.

Here is the missing images.
Attachment #8936773 - Flags: review+ → review?(spenades)
Comment on attachment 8936773 [details]
Bug 1415545 - Land images related to performance documentation page in DevTools.

https://reviewboard.mozilla.org/r/207484/#review218228
Attachment #8936773 - Flags: review?(spenades) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/717d16330a1b
Land images related to performance documentation page in DevTools. r=sole
https://hg.mozilla.org/mozilla-central/rev/717d16330a1b
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.