Performance Improvements #217

majapw · 2016-12-19T13:48:48Z

I'm marking this as breaking due to the removal of the modifiers prop from CalendarDay. It's a fairly minor change all in all, because it's not really being used at all throughout the library, but nevertheless breaking.

This does a number of small performance improvements:

Opening the DRP/SDP no longer causes 4 renders (I had restructured the OutsideClickHandler logic when the tether was introduced and this basically reverts to the pre-tether world).
The DateInput only has an onFocus action instead of an onFocus action on the input and an onClick on the input's wrapper. Previously, when we were disabling the input on touch screens, this was a necessary action. However, readonly inputs do still receive focus events on touch devices afaict so this is no longer necessary.
The CalendarMonthGrid component now keeps track of the month moment objects used for its children in its state and only updates them as necessary (instead of creating new moment objects on every render).
The CalendarMonth component also now keeps track of the day moment objects used for its children in its state and only updates them as necessary.
The CalendarDay no longer takes a modifiers prop. It was only using these props in its user interaction callbacks and it was hard not to rerender each time as the modifiers array was being reinstantiated each time.

I used react-addons-perf to track the improvements. The following wasted time printouts are from 5 seconds of basic interaction with the component.

Before:

After:

There's likely still some work that can be done, but this seems like a good start.

Should help with #178 and #140

to: @airbnb/webinfra @ljharb @lencioni

coveralls · 2016-12-19T16:01:42Z

Coverage decreased (-1.6%) to 84.916% when pulling 3d90a46 on maja-perf-improvements into 08906d6 on master.

ljharb · 2016-12-19T16:52:49Z

src/components/CalendarDay.jsx

-  handleDayClick(day, modifiers, e) {
-    this.props.onDayClick(day, modifiers, e);
+  onDayClick(day, e) {
+    this.props.onDayClick(day, e);


Not a big deal, but it'd be good to destructure out the prop and then call it, so that its this value won't be this.props (same throughout)

ljharb · 2016-12-19T16:55:45Z

src/components/CalendarMonthGrid.jsx

+    const hasNumberOfMonthsChanged = this.props.numberOfMonths !== numberOfMonths;
+    if (hasMonthChanged || hasNumberOfMonthsChanged) {
+      if (initialMonth.isAfter(this.props.initialMonth)) {
+        months.shift();


This is mutating the state array directly, rather than using setState - is this intentional? Can we avoid the mutation?

This definitely needs to be changed. You should use the callback version of setState:

this.setState((previousState) => { ... return {...}; });

What do you mean by using that callback @lencioni? How is that more helpful than just calling setState?

setState is async, so if you need to modify state based on the previous state, you need to use the callback version of setState otherwise it may end up doing an incorrect thing, which could be a subtle cause of bugs.

Of course, if you don't need anything from the previous state, then you don't need to use the callback version.

This is in componentWillReceiveProps and is directly mutating the current state which might be a problem, but is doing so based on the change in props not on a change in state.

I don't think we need the callback, but I can figure out a way to do this with setState instead of mutating the state under the hood.

ljharb · 2016-12-19T16:56:28Z

src/components/CalendarMonthGrid.jsx

+            (i >= firstVisibleMonthIndex) && (i < firstVisibleMonthIndex + numberOfMonths);
+          return (
+            <CalendarMonth
+              key={`CalendarMonth-${i}`}


This key just needs to be i - ie, only unique within the array.

i thought we were against using indices as keys

Ideally this would not include the index at all and would be based off of unique datae. But what you have here is functionally equivalent to a plain index with unnecessarily added complexity of the string.

ljharb · 2016-12-19T16:57:37Z

src/components/DateRangePicker.jsx

@@ -263,32 +265,36 @@ export default class DateRangePicker extends React.Component {
      onFocusChange,
    } = this.props;

+    const onOutsideClick = !withPortal && !withFullScreenPortal ? this.onOutsideClick : () => {};


It would be more performant to cache this noop function outside the render path, so react can know when it's the same object. Alternatively, set it to null/undefined, and it won't be passed at all?

ljharb · 2016-12-19T16:58:00Z

src/components/SingleDatePicker.jsx

@@ -325,27 +325,31 @@ export default class SingleDatePicker extends React.Component {

    const dateString = this.getDateString(date);

+    const onOutsideClick = !withPortal && !withFullScreenPortal ? this.onOutsideClick : () => {};


Same comment here re noop vs undefined

lencioni · 2016-12-19T17:45:34Z

src/components/CalendarDay.jsx

+        onMouseUp={(e) => this.onDayMouseUp(day, e)}
+        onClick={(e) => this.onDayClick(day, e)}
+        onTouchStart={(e) => this.onDayTouchStart(day, e)}
+        onTouchEnd={(e) => this.onDayTouchEnd(day, e)}


If you are going for performance, you might also want to consider binding these event handlers in the constructor, and then re-binding only when this.props.day changes.

oh actually, that's a good point - they can be bound in the constructor and simply look up this.props.day every time, so there's no need for any rebinding.

Ah, yes of course!

(they actually could have done this in the first place, too)

lencioni · 2016-12-19T17:47:39Z

src/components/CalendarMonth.jsx

+    });
+
+    return (
+      <div className={calendarMonthClasses} data-visible={isVisible}>


unrelated: in addition to data-visible (or maybe instead of it?) we might want to use aria-hidden.

Probably not in this PR, but you are correct

lencioni · 2016-12-19T17:48:52Z

src/components/CalendarMonth.jsx

+
+          <tbody className="js-CalendarMonth__grid">
+            {weeks.map((week, i) =>
+              <tr key={i}>


Array index keys should generally be avoided. I think it might be okay in this case, but it would still be better to use something that identifies the week object instead if that is possible.

lencioni · 2016-12-19T17:49:31Z

src/components/CalendarMonth.jsx

+                  }, modifiersForDay.map(mod => `CalendarMonth__day--${mod}`));
+
+                  return (
+                    <td className={className} key={j}>


Same comment here regarding array index and j.

The comment on CalendarMonthGrid suggests that this is preferable and acceptable. What is the change you are proposing?

day is some sort of moment object, right? I'd go with something like this key={day.dayOfYear()}. I think it might be more important for the tr to have a better key than it is for this td to have a better key, but you might as well make them all better at the same time.

It is not always a moment object. Sometimes it is null and also @ljharb mentioned that these only have to be unique within the array, no?

yes, key only needs to be unique within the array. That said, when it is a moment object, making it something actually unique and tied to the row might make things more performant, even if some of the items use an index.

Since some of the day variables may equal null, how do you propose using something different for the moment objects and not having it collide with the keys for the null values? I think having it tied to day of week makes the most sense.

Day of the week, and index, seem like they wouldn't conflict - alternatively, 'day_' + dayNumber wouldn't collide either.

j is already day of the week, like it goes from 0 - 6. I guess i don't understand what the problem is with this key...

in that case maybe the only problem is the variable name :-)

lencioni · 2016-12-19T17:54:50Z

src/components/CalendarMonth.jsx

+  }
+
+  shouldComponentUpdate(nextProps, nextState) {
+    return shallowCompare(this, nextProps, nextState);


How much benefit does this give you now that you've moved the moment objects to state?

It still diminishes the render count significantly so I think it is meaningful to leave this.

lencioni · 2016-12-19T17:56:43Z

src/components/CalendarMonthGrid.jsx

+    const hasNumberOfMonthsChanged = this.props.numberOfMonths !== numberOfMonths;
+    if (hasMonthChanged || hasNumberOfMonthsChanged) {
+      if (initialMonth.isAfter(this.props.initialMonth)) {
+        months.shift();


This definitely needs to be changed. You should use the callback version of setState:

this.setState((previousState) => { ... return {...}; });

lencioni · 2016-12-19T17:58:39Z

src/components/CalendarMonthGrid.jsx

@@ -95,11 +115,22 @@ export default class CalendarMonthGrid extends React.Component {
    this.props.onMonthTransitionEnd();
  }

+  getMonths(initialMonth, numberOfMonths) {
+    let month = initialMonth.clone().subtract(1, 'month');


As this function is written, it is a little difficult to grok. To avoid doing mental acrobatics, what do you think about moving this into your loop? It shouldn't be too hard if methods like add or subtract can take negative numbers.

sontek · 2016-12-28T00:37:09Z

@majapw Are you still working on this? We just finished our implementation of react-dates but are holding off on shipping it live because of the bad performance even on iphone6. I think this PR might solve that for us

I'm willing to test this and give feedback if you ship it up as @next or something as well

sontek · 2016-12-28T18:42:56Z

@majapw Also, I noticed airbnb just uses another picker on mobile. Is that because of performance or were there other criteria they used when doing that?

Wondering if I should be following the same workflow?

ljharb · 2016-12-28T18:44:21Z

@sontek any place we're not using react-dates is because we haven't migrated it over yet (i.e., legacy)

majapw · 2017-01-12T18:25:54Z

Hi @sontek! Sorry for the delay! I've been on vacation. I'll be returning to this this week and hopefully shipping it. :)

coveralls · 2017-01-13T02:04:37Z

Coverage decreased (-1.6%) to 85.534% when pulling 89b3dac on maja-perf-improvements into f931a82 on master.

sontek · 2017-01-22T06:59:07Z

@majapw No worries on the delay, everyone deserves a vacation! :) I'm just excited to see work on the performance since its the only thing holding us back on using this!

sontek · 2017-01-23T16:14:14Z

@majapw Would it make sense to break up some of these changes into smaller pieces so it'll be easier to get some of the changes into a release?

coveralls · 2017-01-24T22:05:44Z

Coverage decreased (-1.5%) to 85.694% when pulling 8f59f88 on maja-perf-improvements into dd5def0 on master.

timhwang21 · 2017-01-25T19:37:47Z

src/components/CalendarMonth.jsx

+            {weeks.map((week, i) => (
+              <tr key={i}>
+                {week.map((day, j) => {
+                  const modifiersForDay = getModifiersForDay(modifiers, day);


I'm not sure how much you guys like comments from randoms on your PRs, but I'm just wondering: as I mentioned in #245, is it possible to run the calculations for days conditionally? As I understand it, even months not currently displayed are having their modifiers calculated. Is there any reason this isn't being done just for visible months? Thanks!

If you don't run the calculations for non-visible months, you get weird behavior for transitions. For example, if part of the selected range is in the month before or the month after, it will look unselected during the transition and then flash to the selected teal color.

Would visible months ±1 on either side work then? I don't imagine many use cases would want to display more than 3 calendars at once, and running the calculations for 5 calendars sounds more appealing than all 12.

That is the current behavior @timhwang21

majapw · 2017-01-25T20:53:38Z

@ljharb @lencioni I have addressed all comments. Can one of you give me a stamp? I'd like to get this merged in today.

coveralls · 2017-01-25T21:07:27Z

Changes Unknown when pulling 769ffda on maja-perf-improvements into ** on master**.

majapw · 2017-01-25T21:14:29Z

Almost all of the additions to the code are in lifecycles methods. I think I'm going to invest some time into getting the full mount tests working, but I'll probably merge through this coveralls check.

ljharb · 2017-01-25T21:45:46Z

src/components/DateRangePicker.jsx

@@ -186,7 +192,7 @@ export default class DateRangePicker extends React.Component {
    } = this.props;
    const { dayPickerContainerStyles } = this.state;

-    const onOutsideClick = !withFullScreenPortal ? this.onOutsideClick : undefined;
+    const onOutsideClick = !withFullScreenPortal && withPortal ? this.onOutsideClick : undefined;


This is fine but not wrapping the condition in parens makes me nervous.

ljharb · 2017-01-25T21:45:50Z

src/components/DateRangePicker.jsx

@@ -263,32 +265,36 @@ export default class DateRangePicker extends React.Component {
      onFocusChange,
    } = this.props;

+    const onOutsideClick = !withPortal && !withFullScreenPortal ? this.onOutsideClick : undefined;


ljharb · 2017-01-25T21:46:04Z

src/components/SingleDatePicker.jsx

@@ -276,7 +276,7 @@ export default class SingleDatePicker extends React.Component {
      selected: day => this.isSelected(day),
    };

-    const onOutsideClick = !withFullScreenPortal ? this.onClearFocus : undefined;
+    const onOutsideClick = !withFullScreenPortal && withPortal ? this.onClearFocus : undefined;


ljharb · 2017-01-25T21:46:10Z

src/components/SingleDatePicker.jsx

@@ -336,27 +336,31 @@ export default class SingleDatePicker extends React.Component {

    const dateString = this.getDateString(date);

+    const onOutsideClick = !withPortal && !withFullScreenPortal ? this.onOutsideClick : undefined;


coveralls · 2017-01-25T21:50:36Z

Coverage decreased (-2.4%) to 84.753% when pulling 769ffda on maja-perf-improvements into d01cd54 on master.

…resenting each month

…ndarMonth Previously, all these day moment objects were being created every time the CalendarMonth was updated. Now, only those CalendarDay objects that have changed are getting updated.

…n you focus on one of the inputs

It was not especially being used, and this prevents the CalendarDay from being rerendered at every opportunity.

coveralls · 2017-01-25T22:23:24Z

Changes Unknown when pulling df18385 on maja-perf-improvements into ** on master**.

coveralls · 2017-01-25T22:41:36Z

Changes Unknown when pulling df18385 on maja-perf-improvements into ** on master**.

coveralls · 2017-01-25T23:17:25Z

Coverage decreased (-2.4%) to 84.815% when pulling df18385 on maja-perf-improvements into 1cbc4db on master.

majapw added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Dec 19, 2016

majapw requested review from ljharb and lencioni December 19, 2016 13:48

majapw force-pushed the maja-perf-improvements branch 2 times, most recently from 426840c to 3d90a46 Compare December 19, 2016 14:55

ljharb requested changes Dec 19, 2016

View reviewed changes

lencioni reviewed Dec 19, 2016

View reviewed changes

majapw mentioned this pull request Dec 22, 2016

DateRangePicker onChange callback #117

Open

majapw mentioned this pull request Jan 12, 2017

onOutsideRange constantly and needlessly recomputes thousands of time #245

Closed

majapw force-pushed the maja-perf-improvements branch from 3d90a46 to 89b3dac Compare January 13, 2017 01:12

majapw force-pushed the maja-perf-improvements branch from 89b3dac to 8f59f88 Compare January 24, 2017 21:48

timhwang21 reviewed Jan 25, 2017

View reviewed changes

majapw force-pushed the maja-perf-improvements branch 2 times, most recently from 5a4c1b2 to 769ffda Compare January 25, 2017 20:51

ljharb approved these changes Jan 25, 2017

View reviewed changes

majapw force-pushed the maja-perf-improvements branch 2 times, most recently from b89c194 to d34207f Compare January 25, 2017 22:05

ljharb approved these changes Jan 25, 2017

View reviewed changes

Maja Wichrowska added 5 commits January 25, 2017 14:16

Updates the CalendarMonthGrid to keep track of the moment objects rep…

e6c6700

…resenting each month

Maintains an array of moment objects reperesenting the days in a Cale…

c4fb866

…ndarMonth Previously, all these day moment objects were being created every time the CalendarMonth was updated. Now, only those CalendarDay objects that have changed are getting updated.

Fix onFocus logic so that the DateRangePicker only rerenders once whe…

15d196b

…n you focus on one of the inputs

Removes the modifiers property from the CalendarDay component

82ade53

It was not especially being used, and this prevents the CalendarDay from being rerendered at every opportunity.

Small fixes to the performance updates

df18385

majapw force-pushed the maja-perf-improvements branch from d34207f to df18385 Compare January 25, 2017 22:17

majapw merged commit 351eb5a into master Jan 25, 2017

majapw deleted the maja-perf-improvements branch January 25, 2017 23:20

		@@ -325,27 +325,31 @@ export default class SingleDatePicker extends React.Component {

		const dateString = this.getDateString(date);

		const onOutsideClick = !withPortal && !withFullScreenPortal ? this.onOutsideClick : () => {};

		@@ -336,27 +336,31 @@ export default class SingleDatePicker extends React.Component {

		const dateString = this.getDateString(date);

		const onOutsideClick = !withPortal && !withFullScreenPortal ? this.onOutsideClick : undefined;

Performance Improvements #217

Performance Improvements #217

Conversation

majapw commented Dec 19, 2016 • edited Loading

coveralls commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sontek commented Dec 28, 2016 • edited Loading

sontek commented Dec 28, 2016

ljharb commented Dec 28, 2016

majapw commented Jan 12, 2017

coveralls commented Jan 13, 2017

sontek commented Jan 22, 2017

sontek commented Jan 23, 2017

coveralls commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw commented Jan 25, 2017

coveralls commented Jan 25, 2017

majapw commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 25, 2017

coveralls commented Jan 25, 2017

coveralls commented Jan 25, 2017

coveralls commented Jan 25, 2017

majapw commented Dec 19, 2016 •

edited

Loading

sontek commented Dec 28, 2016 •

edited

Loading