Optimize deleteModifier/addModifier #1659
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lencioni
added
semver-patch: fixes/refactors/etc
lencioni
force-pushed
the
jdl-reduce
branch
2 times, most recently
from
f9fb3c3 to
6394c11
Compare
lencioni
force-pushed
the
jdl-reduce
branch
2 times, most recently
from
479f4d4 to
9cd6c77
Compare
|
Alright friends, tests are passing and I think this is ready for review! |
lencioni
added a commit
that referenced
this pull request
May 30, 2019
After the optimizations I applied to generating modifiers in #1659, I noticed that about 50% of the time in componentWillReceiveProps is spent in isDayVisible() doing moment operations to generate the boundaries for the check. Since these boundaries remain very stable across each call, we can reduce most of the work done here to very little by adding some caching of these values. On top of my reduce optimizations, this brings down the time spent in componentWillReceiveProps to ~15ms. I'm not doing anything to limit the size of these Maps, so this will leak some memory, but I think the size of the cache will be really small in most cases, so I'm not sure it matters.
lencioni
added a commit
that referenced
this pull request
May 30, 2019
After the optimizations I applied to generating modifiers in #1659, I noticed that about 50% of the time in componentWillReceiveProps is spent in isDayVisible() doing moment operations to generate the boundaries for the check. Since these boundaries remain very stable across each call, we can reduce most of the work done here to very little by adding some caching of these values. On top of my reduce optimizations, this brings down the time spent in componentWillReceiveProps to ~15ms. I'm not doing anything to limit the size of these Maps, so this will leak some memory, but I think the size of the cache will be really small in most cases, so I'm not sure it matters.
lencioni
added a commit
that referenced
this pull request
May 30, 2019
After the optimizations I applied to generating modifiers in #1659, I noticed that about 50% of the time in componentWillReceiveProps is spent in isDayVisible() doing moment operations to generate the boundaries for the check. Since these boundaries remain very stable across each call, we can reduce most of the work done here to very little by adding some caching of these values. On top of my reduce optimizations, this brings down the time spent in componentWillReceiveProps to ~15ms. I'm not doing anything to limit the size of these Maps, so this will leak some memory, but I think the size of the cache will be really small in most cases, so I'm not sure it matters.
While profiling DayRangePickerController, I noticed that we spend a significant amount of time generating modifiers whenever anything is changed. A good amount of this cost comes from these reducers that unnecessarily create new objects and spread into them on every iteration. We can make this faster by mutating the accumulator while inside the reducer. In my profiling, this reduces the speed of deleteModifier from ~28ms to ~20ms, and I think it will reduce the memory usage, which should further help reduce garbage collection times.
We create this object in this function and then return it at the end, so it should be safe to just mutate it while we are in this function instead of rebuilding it immediately again. This reduces the amount of time spent in deleteModifier from ~20ms to ~16ms.
I noticed that we always create a new set and then delete from it even if the modifier is not in the set, or create a new set and then add it even it the modifier is already in the set. This optimization reduces the cost of DayPickerRangeController#componentWillReceiveProps from ~16ms to ~12ms.
esprehn
approved these changes
May 31, 2019
ljharb
reviewed
May 31, 2019
| [toISODateString(startOfMonth)]: [], | ||
| [toISODateString(startOfMonth.clone().add(1, 'day'))]: [], | ||
| [toISODateString(startOfMonth.clone().add(2, 'days'))]: [], | ||
| [toISODateString(startOfMonth)]: new Set(), |
There was a problem hiding this comment.
this type change suggests an observable api change - is that the case, or am i missing context?
There was a problem hiding this comment.
I could be wrong but I think this is just a care of these fixtures being incorrect. Previously they were being converted to sets in the code by happenstance, but I think all of there are sets throughout the actual code.
There was a problem hiding this comment.
On phone now but I think these might be able to be passed in as props, so I might need to add a conversation step somewhere like in the constructor. I'll dig in more tomorrow.
There was a problem hiding this comment.
Nope, as far as I can tell, this is all component state. These tests are injecting it into the component by calling setState directly.
This is in the constructor where this variable begins its life: https://github.com/airbnb/react-dates/blob/0e2ed9a322dd0d8f9782614b40a362413406a76b/src/components/DayPickerRangeController.jsx#L217
And getStateForNewMonth uses getModifiers for visibleDates, which always uses Set for the set of modifiers: https://github.com/airbnb/react-dates/blob/0e2ed9a322dd0d8f9782614b40a362413406a76b/src/components/DayPickerRangeController.jsx#L965-L971
I'm pretty sure this is all internal stuff.
ljharb
approved these changes
May 31, 2019
|
Yeah, it seems that there's a fair amount of duplicated code here, so abstracting something might make sense. |
devs-cloud
pushed a commit
to devs-cloud/react-date
that referenced
this pull request
Dec 27, 2019
After the optimizations I applied to generating modifiers in react-dates/react-dates#1659, I noticed that about 50% of the time in componentWillReceiveProps is spent in isDayVisible() doing moment operations to generate the boundaries for the check. Since these boundaries remain very stable across each call, we can reduce most of the work done here to very little by adding some caching of these values. On top of my reduce optimizations, this brings down the time spent in componentWillReceiveProps to ~15ms. I'm not doing anything to limit the size of these Maps, so this will leak some memory, but I think the size of the cache will be really small in most cases, so I'm not sure it matters.
MarcoAntonioAG
pushed a commit
to MarcoAntonioAG/React-dates
that referenced
this pull request
Mar 31, 2022
After the optimizations I applied to generating modifiers in react-dates/react-dates#1659, I noticed that about 50% of the time in componentWillReceiveProps is spent in isDayVisible() doing moment operations to generate the boundaries for the check. Since these boundaries remain very stable across each call, we can reduce most of the work done here to very little by adding some caching of these values. On top of my reduce optimizations, this brings down the time spent in componentWillReceiveProps to ~15ms. I'm not doing anything to limit the size of these Maps, so this will leak some memory, but I think the size of the cache will be really small in most cases, so I'm not sure it matters.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While profiling DayRangePickerController, I noticed that we spend a
significant amount of time generating modifiers whenever anything is
changed. A good amount of this cost comes from these reducers that
unnecessarily create new objects and spread into them on every
iteration. We can make this faster by mutating the accumulator while
inside the reducer.
In my profiling, this reduces the speed of deleteModifier from ~28ms to
~20ms, and I think it will reduce the memory usage, which should further
help reduce garbage collection times.
Mutate updatedDaysAfterDeletion in deleteModifier/addModifier
We create this object in this function and then return it at the end, so
it should be safe to just mutate it while we are in this function
instead of rebuilding it immediately again.
This reduces the amount of time spent in deleteModifier from ~20ms to
~16ms.
Avoid creating new Sets when deleting/adding modifiers
I noticed that we always create a new set and then delete from it even
if the modifier is not in the set, or create a new set and then add it
even it the modifier is already in the set. This optimization reduces
the cost of DayPickerRangeController#componentWillReceiveProps from
~16ms to ~12ms.
Before:
After:
