Perf rearchitecting #450

majapw · 2017-04-14T21:42:46Z

This PR is a dramatic rearchitecture of the way that we handle modifiers in react-dates on the whole.

Instead of passing down an object of modifier string => modifiers functions to each CalendarDay component (one that gets rebuilt on every render call) and expecting the CalendarDay component to take care of updating itself, we now put the burden on the top of the tree instead of on the leaves. The previous model meant that whenever an interaction happened on an individual day, even a hover interaction, every single CalendarDay would have to recalculate its modifiers and rerender as a result. The idea was that an interaction on one day might have effects elsewhere in the calendar (like the hovered-span). The effect unfortunately was that react-dates was really god damn slow.

In this new model, the DayPickerRangeController maintains a map with the following structure:

{
  MONTH_ISO_1: {
    DAY_ISO_1: Set(['modifier_1', 'modifier_2', ...]),
    DAY_ISO_2: Set(['modifier_1', 'modifier_2', ...]),
    ...
  },
  ...
}

It passes this down the tree such that each CalendarMonth and each CalendarDay only gets the information that pertains to it. This means that the updating of these modifiers is also handled at the top-level and is done in the componentWillReceiveProps, onDayMouseEnter, and onDayMouseLeave methods of the DayPickerRangeController. Fortunately, this allows us to more finely tune which days get updated and speeds up the rerendering/updating process dramatically.

I did some preliminary testing and saw the following improvements:
Before:

After:

I will update those graphs when everything is in place.

I still need to finish writing some tests, clean up some other tests, and also get this working with enableOutsideDays and the SingleDatePicker. The bulk of the work is in DayPickerRangeController right now, but I've been musing on ways to pull that in an HOC or something. SUGGESTIONS ARE WELCOME.

In any case, I would appreciate a first review pass while I finish up the above points! <3
@lencioni @ljharb @moonboots @backwardok

UPDATE: Here are the final perf measurements!
Before:

After:

Here are the findings:

The overhead on initial mount remains about the same (~400-500ms)
Any calendar interaction (hover, click, etc.) consistently caused a 65ms update in the old version and now only takes about 10ms - 15ms.
Transitions are now a bit slower (likely due to a bug in the code and therefore an opportunity) at about 120ms as opposed to 95ms previously. There does seem to be an unnecessary update occurring that I'll investigate in a follow up PR.

coveralls · 2017-04-14T21:53:34Z

Coverage decreased (-3.5%) to 80.497% when pulling 6faae02 on maja-prototype-perf-idea into 3815516 on master.

coveralls · 2017-04-18T21:23:33Z

Coverage decreased (-56.8%) to 27.181% when pulling 5473b4f on maja-prototype-perf-idea into 3815516 on master.

coveralls · 2017-04-18T21:58:12Z

Coverage increased (+2.6%) to 86.593% when pulling 37eafdd on maja-prototype-perf-idea into 3815516 on master.

coveralls · 2017-04-18T21:58:12Z

Coverage increased (+2.6%) to 86.593% when pulling 37eafdd on maja-prototype-perf-idea into 3815516 on master.

coveralls · 2017-04-18T22:16:28Z

Coverage increased (+2.6%) to 86.593% when pulling 37eafdd on maja-prototype-perf-idea into 3815516 on master.

ljharb · 2017-04-18T22:16:54Z

src/components/CalendarDay.jsx

@@ -16,7 +16,7 @@ const propTypes = forbidExtraProps({
  day: momentPropTypes.momentObj,
  daySize: nonNegativeInteger,
  isOutsideDay: PropTypes.bool,
-  modifiers: PropTypes.object,
+  modifiers: PropTypes.instanceOf(Set),


it's worth noting that this may make the PR semver-major, as this adds a requirement for es6-shimmed or above, and breaks IE <= 8 (if it wasn't already broken)

This PR is already semver-major due to the fact that the DayPicker now functions totally differently. Do you think this is a concern? I kind of think this project was not working with IE8 already.

As long as the PR is already semver-major, and as long as we ensure that these requirements (Set and Array.from being globally polyfilled) are documented in the readme, I'm a solid 👍 on it.

ljharb · 2017-04-18T22:17:57Z

src/components/CalendarDay.jsx

    const className = cx('CalendarDay', {
      'CalendarDay--outside': isOutsideDay,
-    }, modifiersForDay.map(mod => `CalendarDay--${mod}`));
-
+    }, [...modifiers].map(mod => `CalendarDay--${mod}`));


Array.from(modifiers, mod => CalendarDay--${mod}) is much much more efficient because it avoids the intervening array.

okeedoke. Didn't know that syntax!

coveralls · 2017-04-24T18:13:33Z

Coverage increased (+3.0%) to 86.961% when pulling c3b5350 on maja-prototype-perf-idea into 3815516 on master.

lencioni · 2017-04-24T18:27:38Z

src/utils/isAfterDay.js

+export default function isAfterDay(a, b) {
+  if (!moment.isMoment(a) || !moment.isMoment(b)) return false;
+
+  const isSameYear = a.year() === b.year();


You might be able to squeeze out a bit more perf by storing the results of x.year/month/date, depending on how expensive those calls are and how often this function is called.

lencioni · 2017-04-24T18:28:31Z

src/utils/isDayVisible.js

+  const firstDayOfFirstMonth = month.clone().startOf('month');
+  const lastDayOfLastMonth = month.clone().add(numberOfMonths - 1, 'months').endOf('month');
+
+  return !day.isBefore(firstDayOfFirstMonth) && !day.isAfter(lastDayOfLastMonth);


Should this use your isBeforeDay/isAfterDay utility methods?

lencioni · 2017-04-24T18:29:21Z

src/utils/isDayVisible.js

+// TODO(maja): get this working for enableOutsideDays
+export default function isDayVisible(day, month, numberOfMonths) {
+  const firstDayOfFirstMonth = month.clone().startOf('month');
+  const lastDayOfLastMonth = month.clone().add(numberOfMonths - 1, 'months').endOf('month');


You can avoid this work some of the time by doing an early return and putting this after it.

coveralls · 2017-04-25T18:03:15Z

Coverage increased (+4.6%) to 88.576% when pulling 04a1e5e on maja-prototype-perf-idea into 3815516 on master.

coveralls · 2017-04-25T21:54:45Z

Coverage increased (+4.7%) to 88.673% when pulling 9220ddb on maja-prototype-perf-idea into 3815516 on master.

ljharb · 2017-04-25T22:26:43Z

src/components/SingleDatePicker.jsx

+      selected: day => this.isSelected(day),
+    };
+
+    const initialVisibleMonthThunk = props.initialVisibleMonth || (() => (props.date || moment()));


this could be more efficient with props.initialVisibleMonth || (props.date ? () => props.date : () => moment()) (and even more so if the moment thunk was cached at module level instead of recreated every render)

ljharb · 2017-04-25T22:27:25Z

src/components/SingleDatePicker.jsx

+    const initialVisibleMonthThunk = props.initialVisibleMonth || (() => (props.date || moment()));
+    const evaluatedMonth = initialVisibleMonthThunk();
+    const currentMonth = moment.isMoment(evaluatedMonth) ? evaluatedMonth : moment();
+    const visibleDays = getVisibleDays(currentMonth, props.numberOfMonths, props.enableOutsideDays);


also, all this prop-dependent logic needs to be in both the constructor and componentWillReceiveProps. Rather than duplicating it, is there some chunk of it that could be in a shared function?

I think there is a problem with the this.props/props paradigm. Also do you mean that this needs to be reset when the month changes?

What I mean is, any prop-dependent logic in any constructor needs to be re-executed in componentWillReceiveProps - so in this case, yes, when numberOfMonths or enableOutsideDays changes, visibleDays and thus this.state.visibleDays would need to be recomputed.

ljharb · 2017-04-25T22:36:13Z

src/components/SingleDatePicker.jsx

+    if (this.isTouchDevice || !hoverDate) return;
+
+    let modifiers = {};
+    modifiers = this.deleteModifier(modifiers, hoverDate, 'hovered');


const modifiers = this.deleteModifier({}, hoverDate, 'hovered');?

altho this looks kind of weird, why the empty object?

The empty object is so that when we call addModifier/deleteModifier the effects stack rather than overwriting each other. So for instance if something happens that causes two different modifiers to be added, we spread everything onto this object and then set the state at the end.

Thanks, that makes sense.

ljharb · 2017-04-25T22:41:39Z

src/utils/isAfterDay.js

@@ -0,0 +1,18 @@
+import moment from 'moment';
+
+export default function isAfterDay(a, b) {


it seems unfortunate to have both isAfterDay and isBeforeDay - would it be worth it to make one of the implementations (say this one) into return !isBeforeDay(a, b) && !isSameDay(a, b);?

we could also probs update usage to only have one. I just thought it was a bit clear to use one over the other in some cases.

I don't mean, let's get rid of both - I mean, let's implement one in terms of the other.

ljharb · 2017-04-25T22:42:05Z

src/utils/isDayVisible.js

+  if (isBeforeDay(day, firstDayOfFirstMonth)) return false;
+
+  const lastDayOfLastMonth = month.clone().add(numberOfMonths - 1, 'months').endOf('month');
+  if (isAfterDay(day, lastDayOfLastMonth)) return false;


return !isAfterDay(day, lastDayOfLastMonth);?

ljharb · 2017-04-25T22:42:29Z

src/utils/isInclusivelyBeforeDay.js


 export default function isInclusivelyBeforeDay(a, b) {
  if (!moment.isMoment(a) || !moment.isMoment(b)) return false;
-  return a.isBefore(b) || isSameDay(a, b);
+  return isBeforeDay(a, b) || isSameDay(a, b);


isn't this return !isAfterDay(a, b);?

ljharb · 2017-04-25T22:42:38Z

src/utils/isInclusivelyAfterDay.js


 export default function isInclusivelyAfterDay(a, b) {
  if (!moment.isMoment(a) || !moment.isMoment(b)) return false;
-  return a.isAfter(b) || isSameDay(a, b);
+  return isAfterDay(a, b) || isSameDay(a, b);


isn't this return !isBeforeDay(a, b);?

majapw · 2017-05-01T23:43:58Z

@ljharb I think I addressed your concerns! Can you take a look?

coveralls · 2017-05-01T23:50:45Z

Coverage increased (+2.2%) to 86.12% when pulling 3c1bd25 on maja-prototype-perf-idea into 767bebc on master.

ljharb · 2017-05-01T23:50:55Z

src/components/DayPickerRangeController.jsx

@@ -249,7 +629,7 @@ export default class DayPickerRangeController extends React.Component {

  isDayAfterHoveredStartDate(day) {
    const { startDate, endDate, minimumNights } = this.props;
-    const { hoverDate } = this.state;
+    const { hoverDate } = this.state || {};


when would the state not be initialized to an empty object?

When this function is called from the constructor, this.state has not yet been set.

... this same response applies on the other areas too.

It's a weird edge case, but this seemed like the cleanest solution? Like this will never be true in the constructor so I could also just return if this.state is not defined... or figure out some way to skip these modifiers

gotcha (sorry for the multi-comment).

#450 (comment) is what i'd expect to be the solution

ljharb · 2017-05-01T23:54:45Z

src/components/SingleDatePicker.jsx

+    return {
+      ...updatedDays,
+      ...{
+        [monthIso]: {


this spread isn't necessary - you can just do:

return { ...updatedDays, [monthISO]: { ...month, [iso]: modifiers, }, };

ljharb · 2017-05-01T23:54:52Z

src/components/SingleDatePicker.jsx

@@ -344,7 +620,8 @@ export default class SingleDatePicker extends React.Component {
  }

  isHovered(day) {
-    return isSameDay(day, this.state.hoverDate);
+    const { hoverDate } = this.state || {};


when would the state not be initialized to an empty object?

When this function is called from the constructor, this.state has not yet been set.

:-/ hmm - in that case it kind of seems like the meat of it should be extracted out to a pure function in a closure, and have both the constructor and isHovered call into it with the data it needs.

can you clarify that a little bit?

do you mean something like isHovered(day, hoverDate)? I feel like that might be odd given that it'd suddenly be different from every other modifiers method

Hmm - yeah, i can see how that'd be unnecessarily weird. I guess this is fine as-is.

coveralls · 2017-05-02T15:42:55Z

Coverage increased (+2.2%) to 86.12% when pulling 0b20ab4 on maja-prototype-perf-idea into 767bebc on master.

Instead of passing down an object of modifier string => modifiers functions to each CalendarDay component and expecting the CalendarDay component to take care of updating itself, we now put the burden on the top of the tree instead of on the leaves. The previous model basically ended up meaning that whenever an interaction happened on an individual day, even a hover interaction, every single CalendarDay would have to recalculate its modifiers and rerender as a result. This was, as you can imagine, really god damn slow. In this new model, the DayPickerRangeController maintains a map with the following structure: ``` { MONTH_ISO_1: { DAY_ISO_1: Set(['modifer_1', 'modifier_2', ...]), DAY_ISO_2: Set(['modifer_1', 'modifier_2', ...]), ... }, ... } ``` It passes this down the tree such that each `CalendarMonth` and each `CalendarDay` only gets the information that pertains to it. This means that the updating of these modifiers is also handled at the top-level and is done in the `componentWillReceiveProps`, `onDayMouseEnter`, and `onDayMouseLeave` methods of the `DayPickerRangeController`. Fortunately, this allows us to more finely tune which days get updated and speeds up the rerendering/updating process dramatically.

coveralls · 2017-05-02T20:23:16Z

Coverage increased (+2.2%) to 85.737% when pulling c95cbed on maja-prototype-perf-idea into c4bc6d6 on master.

dburles · 2017-06-14T04:59:53Z

Hey guys, I have the following on a DayPicker component in order to highlight a range of dates that contain information, along with highlighting the currently selected day. However it now no longer works after this refactor. Is there another approach I can take to make it work? thanks.

            modifiers={{
              selected: day =>
                moment(this.props.selectedDay).isSame(day, 'day'),
              'highlighted-calendar': day1 =>
                this.props.highlightedDays.some(day2 =>
                  moment(day2).isSame(day1, 'day'),
                ),
            }}

majapw · 2017-06-14T05:06:02Z

Hey @dburles, can you open an issue for this so that we can have a convo on a separate thread?

dburles · 2017-06-14T05:09:00Z

@majapw sure thing! #562

majapw force-pushed the maja-prototype-perf-idea branch from 6faae02 to 5473b4f Compare April 18, 2017 21:10

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

majapw force-pushed the maja-prototype-perf-idea branch 2 times, most recently from 6eb0600 to 37eafdd Compare April 18, 2017 21:55

ljharb reviewed Apr 18, 2017

View reviewed changes

majapw changed the title ~~WIP: Perf rearchitecting~~ Perf rearchitecting Apr 24, 2017

majapw force-pushed the maja-prototype-perf-idea branch from 37eafdd to c3b5350 Compare April 24, 2017 17:54

lencioni previously approved these changes Apr 24, 2017

View reviewed changes

majapw dismissed lencioni’s stale review via 04a1e5e April 25, 2017 17:49

majapw force-pushed the maja-prototype-perf-idea branch from c3b5350 to 04a1e5e Compare April 25, 2017 17:49

majapw force-pushed the maja-prototype-perf-idea branch from 04a1e5e to 9220ddb Compare April 25, 2017 21:34

ljharb reviewed Apr 25, 2017

View reviewed changes

majapw force-pushed the maja-prototype-perf-idea branch 2 times, most recently from b15a284 to 3c1bd25 Compare May 1, 2017 23:37

ljharb previously approved these changes May 1, 2017

View reviewed changes

majapw dismissed ljharb’s stale review via 0b20ab4 May 2, 2017 15:29

majapw force-pushed the maja-prototype-perf-idea branch from 3c1bd25 to 0b20ab4 Compare May 2, 2017 15:29

lencioni previously approved these changes May 2, 2017

View reviewed changes

ljharb previously approved these changes May 2, 2017

View reviewed changes

majapw mentioned this pull request May 2, 2017

Feature / RTL Support #454

Merged

majapw dismissed stale reviews from ljharb and lencioni via c95cbed May 2, 2017 20:02

majapw force-pushed the maja-prototype-perf-idea branch from 0b20ab4 to c95cbed Compare May 2, 2017 20:02

backwardok approved these changes May 2, 2017

View reviewed changes

majapw merged commit 7213805 into master May 2, 2017

majapw deleted the maja-prototype-perf-idea branch May 2, 2017 20:24

This was referenced May 2, 2017

Too slow for mobile #272

Closed

onOutsideRange constantly and needlessly recomputes thousands of time #245

Closed

Need help/hints to debug this behaviour #178

Closed

Lots of issues #140

Closed

dancingshell mentioned this pull request May 9, 2017

Browser support for Safari? #498

Closed

dburles mentioned this pull request Jun 14, 2017

Breaking change to modifiers prop on DayPicker #562

Closed

		@@ -0,0 +1,18 @@
		import moment from 'moment';

		export default function isAfterDay(a, b) {

Perf rearchitecting #450

Perf rearchitecting #450

Conversation

majapw commented Apr 14, 2017 • edited Loading

coveralls commented Apr 14, 2017 • edited Loading

coveralls commented Apr 18, 2017 • edited Loading

coveralls commented Apr 18, 2017

coveralls commented Apr 18, 2017

coveralls commented Apr 18, 2017 • edited Loading

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

coveralls commented Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 25, 2017 • edited Loading

coveralls commented Apr 25, 2017 • edited Loading

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

majapw commented May 1, 2017

coveralls commented May 1, 2017 • edited Loading

Choose a reason for hiding this comment

majapw May 1, 2017 • edited Loading

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

coveralls commented May 2, 2017 • edited Loading

coveralls commented May 2, 2017 • edited Loading

dburles commented Jun 14, 2017 • edited Loading

majapw commented Jun 14, 2017

dburles commented Jun 14, 2017

majapw commented Apr 14, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading

coveralls commented Apr 18, 2017 •

edited

Loading

coveralls commented Apr 18, 2017 •

edited

Loading

coveralls commented Apr 24, 2017 •

edited

Loading

coveralls commented Apr 25, 2017 •

edited

Loading

coveralls commented Apr 25, 2017 •

edited

Loading

coveralls commented May 1, 2017 •

edited

Loading

majapw May 1, 2017 •

edited

Loading

coveralls commented May 2, 2017 •

edited

Loading

coveralls commented May 2, 2017 •

edited

Loading

dburles commented Jun 14, 2017 •

edited

Loading