1390093 - Add whydidyouupdate mixin to React components

Status: RESOLVED WONTFIX

(DevTools :: General, enhancement, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The whydidyouupdate mixin shows whether each render of a component was required or not. It turns out that although each render may only take 20 - 50ms 95% of our renders are unnecessary. If that is e.g. 20 renders then that is up to a second wasted.

The plugin is tweaked a little to give slightly more information.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1390093 - Add whydidyouupdate mixin to React components

Review commit: https://reviewboard.mozilla.org/r/168320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168320/

Comment 2

[Jason Laster [:jlast]]

Comment on attachment 8897038 [details]
Bug 1390093 - Add whydidyouupdate mixin to React components

https://reviewboard.mozilla.org/r/168320/#review174090

Looks good

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8897038 [details]
Bug 1390093 - Add whydidyouupdate mixin to React components

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168320/diff/1-2/

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

A huge part of the reason for the timing mixin was was automated testing using DAMP but Alex has convinced me that timing is unreliable so we can't really use it for tests.

Saying that, 99% of this patch is providing mixins for *all* of our React components so the patch is still very useful.

I will fold the patch from bug 1390092 into the patch and remove the timing stuff so it can be used with whydidyouupdate instead.

I will also quickly create a new mixin that can be used to identify exactly which properties should be added to shouldComponentUpdate().

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8897038 [details]
Bug 1390093 - Add whydidyouupdate mixin to React components

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168320/diff/2-3/

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@jlast: Can you review this again? I only ask because it has changed significantly... you only need to review mixinService.js.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

mixinService.js was completely breaking the JSON viewer and it turned out to be a difficult problem to solve.

The mixinService is added to all our React components. The JSON viewer runs completely in the content process so the mixinService can't be used with it (it needs to use Services.props to access fx properties).

The second problem was that I was getting "source URI is not allowed in this document: resource://devtools/shared/mixinService.js." because the JSON viewer has a null origin.

I removed the mixinService requires from the json viewer but the following files are used by it:

- devtools/client/shared/components/tree/tree-view.js
- devtools/client/shared/components/reps/reps.js
- devtools/client/shared/components/tree/tree-row.js
- devtools/client/shared/components/tree/tree-header.js
- devtools/client/shared/components/tree/tree-cell.js
- devtools/client/shared/components/tabs/tabs.js
- devtools/client/shared/components/tree/label-cell.js

These also require mixinService but we can't remove the requires from these files or it won't work inside the toolbox.

I tried:
- Conditionalizing the require statements but, of course, requirejs parses the script as text and extracts any require statements without taking any account of the code's logic.
- Using "empty:" but it did nothing because we are not running the optimizer.
- Using onBuildWrite to change the module's script.
- Using onBuildRead to change the module's script.

I discussed it with various people but nobodies suggestions worked.

Finally found the solution:

```
define("devtools/shared/mixinService", {
  mixinService: {
    mixins: []
  }
});
```

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8897038 [details]
Bug 1390093 - Add whydidyouupdate mixin to React components

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168320/diff/3-4/

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8897038 [details]
Bug 1390093 - Add whydidyouupdate mixin to React components

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168320/diff/4-5/

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8897038 [details]
Bug 1390093 - Add whydidyouupdate mixin to React components

https://reviewboard.mozilla.org/r/168320/#review192116

::: devtools/client/netmonitor/src/components/request-list-column-domain.js:18
(Diff revision 5)
>  const { L10N } = require("../utils/l10n");
>  const { propertiesEqual } = require("../utils/request-utils");
>  
>  const { div } = DOM;
>  
> +loader.lazyRequireGetter(this, "mixinService", "devtools/shared/mixinService", true);

I'm curious whether `loader.lazyRequireGetter` works when running netmonitor on launchpad? Have you try to run launchpad locally?

See also http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/README.md

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Ricky Chien [:rickychien] (inactive) from comment #10)
> Comment on attachment 8897038 [details]
> I'm curious whether `loader.lazyRequireGetter` works when running netmonitor
> on launchpad? Have you try to run launchpad locally?
> 

No, it wouldn't have worked in Launchpad.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

For the moment we have decided to stick with using perf.html and React 16's excellent markers. If we decide to use HOCs we can always reopen this bug.