[Work in progress] Add reselect and reduce unnecessary renders by asolove · Pull Request #2784 · firefox-devtools/debugger

asolove · 2017-05-02T19:48:54Z

Associated Issue: #2727

Background

In issue #2727, @fflorent reported a very severe performance problem on pages with lots of script tags. In the course of debugging that problem, we noticed that all the components connected to the redux store were being re-rendered on every state change, even if the state change didn't effect the data that component read. That was odd, because Redux promises not to re-render connected components if the props don't change (as tested by shallow equality). The culprit was our selector functions, which often use map and filter to build up results. Even if the underlying data stayed the same, these selectors return new objects and arrays every time they are called. A common solution to this problem is to use memoized selectors, which promise to return the same objects if the underlying data hasn't changed. One popular library for doing this is reselect.

So the goal of this PR is to make components only re-render when their underlying data has changed, by memoizing selectors as necessary.

Summary of Changes

For each component, make it a PureComponent and ensure its selectors don't change if their underlying data doesn't change:

Test Plan

All existing tests should pass (no functionality should have changed).

To verify that re-renders have decreased:

Do a development build so React Perf tools are installed.
Open debugger.html and then open a debugger for it.
Type Perf.start()
Interact with the app as much as you like...
Type Perf.stop()
Perf.printExclusive() will now print a table with how long it spend re-rendering each component. This allows you to see if one component is changing because of unrelated data.

codecov · 2017-05-02T20:00:51Z

Codecov Report

Merging #2784 into master will increase coverage by 0.27%.
The diff coverage is 79.54%.

@@            Coverage Diff             @@
##           master    #2784      +/-   ##
==========================================
+ Coverage   58.74%   59.01%   +0.27%     
==========================================
  Files          59       59              
  Lines        2288     2301      +13     
  Branches      469      470       +1     
==========================================
+ Hits         1344     1358      +14     
+ Misses        944      943       -1

Impacted Files	Coverage Δ
src/selectors.js	`100% <ø> (ø)`	⬆️
src/utils/source.js	`50% <0%> (-2.39%)`	⬇️
src/reducers/expressions.js	`97.95% <100%> (+0.04%)`	⬆️
src/reducers/pause.js	`21.91% <75%> (+7.83%)`	⬆️
src/reducers/sources.js	`78.78% <87.5%> (+0.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c00a0a4...9ff5d29. Read the comment docs.

fflorent · 2017-05-02T20:01:17Z

Thanks @asolove! I may give it a try tomorrow.

Florent

fflorent · 2017-05-02T20:04:31Z

BTW, I see you use babel. Doesn't it introduce performance issues? (especially for Maps and Sets)

Florent

jasonLaster

I like where this is going. I'll review more closely soon

jasonLaster · 2017-05-02T22:31:13Z

+if (process.env.NODE_ENV !== "production") {
+  const Perf = require("react-addons-perf");
+  window.Perf = Perf;
+}


jasonLaster · 2017-05-02T22:32:05Z

-export function getPause(state: OuterState) {
-  return state.pause.get("pause");
-}
+const getPauseWrapper = state => state.pause;


I like this style

jasonLaster · 2017-05-02T22:32:35Z

 // (right now) to type those wrapped functions.
 type OuterState = { sources: Record<SourcesState> };

+const getSourcesWrapper = state => state.sources;


perhaps sourcesState or is that too opaque?

jasonLaster · 2017-05-02T22:33:33Z

-}
+const _getBreakpoints = createSelector(
+  getBreakpoints,
+  state => state,


does this negate the selector?

jasonLaster · 2017-05-03T14:27:58Z

  return state.sources.sources.find(source => source.get("url") === url);
 }

+export function getSourceInSources(sources: I.Map<string, Source>, id: string) {


nit: getSourceByID

Well the problem is we already have a selector getSourceById that takes the full state and an id. This function takes just the actual sources map and an id. For various kinds of callers, we need both.

Yeah - the underscore was meh when it was a private function, but below it's public... so i don't love it

Yeah, not sure what to do without finding a better name for either the top-level "sources state" or the nested "sources by id map".

jasonLaster · 2017-05-03T14:28:29Z

@@ -284,8 +284,12 @@ export function getSourceByURL(state: OuterState, url: string) {
  return state.sources.sources.find(source => source.get("url") === url);


we should have a separate URL to ID mapping

…eed to pass props.

* types * utils

fflorent · 2017-05-03T19:11:23Z

@asolove I heard you also recently made a patch that is not landed yet in Firefox Dev Edition and fixed a big performance issue. And at work, I could notice a really good improvement of the performance already in master using the repository. Thanks a lot for this big effort! :)

Florent

jasonLaster · 2017-05-03T19:18:14Z

@fflorent babel certainly adds a slow down, but it is worth it to have some consistency across runtimes. We don't use it to add experimental language features.

Memoizes selectors to prevent unnecessary renders.

jasonLaster reviewed May 2, 2017

View reviewed changes

jasonLaster reviewed May 3, 2017

View reviewed changes

jasonLaster and others added 12 commits May 3, 2017 13:29

reselect

844b22d

Fix breakpoint selector.

532a435

Add react-addons-perf to search for wasted renders.

e4ee60b

Make Editor a PureComponent and avoid unnecessary CodeMirror calls.

e9e05b5

Editor selector usage avoids wasted renders.

51512f6

Fix Expressions component re-rendering unnecessarily.

6ce1af3

Fix SourceTabs component from re-rendering unnecessarily.

f17cf66

Fix App re-rendering unnecessarily. Sources fetches data itself, no n…

5fa7791

…eed to pass props.

Remove Scopes' custom shouldComponentUpdate & memoize selectors instead.

b1494cf

Snapshot changes were wrong. Just due to local changes.

b32bfcb

Improve Breakpoints component selector.

aca49f0

Small tweaks

4d7be47

* types * utils

jasonLaster force-pushed the reselect2 branch from a56bae8 to 4d7be47 Compare May 3, 2017 17:36

jasonLaster added 4 commits May 3, 2017 14:36

further tweaks

2545022

tweaks

983533f

additional tweaks

2e93673

re-order functions

9ff5d29

jasonLaster merged commit cdcb10b into firefox-devtools:master May 3, 2017

DanUgelow pushed a commit to DanUgelow/debugger.html that referenced this pull request May 4, 2017

Add reselect and reduce unnecessary renders (firefox-devtools#2784)

cb860ef

Memoizes selectors to prevent unnecessary renders.

		@@ -284,8 +284,12 @@ export function getSourceByURL(state: OuterState, url: string) {
		return state.sources.sources.find(source => source.get("url") === url);

Conversation

asolove commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Summary of Changes

Test Plan

Uh oh!

codecov Bot commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fflorent commented May 2, 2017

Uh oh!

fflorent commented May 2, 2017

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fflorent commented May 3, 2017

Uh oh!

jasonLaster commented May 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asolove commented May 2, 2017 •

edited

Loading

codecov Bot commented May 2, 2017 •

edited

Loading