[1.x] [Labs] Add useManualCalc prop to TimezonePicker as speed optimization #2564
Conversation
| export function getInitialTimezoneItems( | ||
| date: Date, | ||
| includeLocalTimezone: boolean, | ||
| useManualCalc: boolean, |
There was a problem hiding this comment.
Worth setting useManualCalc: boolean = false? Then you don't have to update all those tests with the extra param.
| return includeLocalTimezone && local !== undefined ? [local, ...populous] : populous; | ||
| } | ||
|
|
||
| /** | ||
| * Get the timezone item for the user's local timezone. | ||
| * @param date the date to use when determining timezone offsets | ||
| */ | ||
| export function getLocalTimezoneItem(date: Date): ITimezoneItem | undefined { | ||
| export function getLocalTimezoneItem(date: Date, useManualCalc: boolean): ITimezoneItem | undefined { |
| @@ -60,6 +60,12 @@ export interface ITimezonePickerProps extends IProps { | |||
| */ | |||
| showLocalTimezone?: boolean; | |||
|
|
|||
| /** | |||
| * Whether to use manual calculation (faster, but possibly less accurate) rather than moment-timezone to infer metadata; | |||
There was a problem hiding this comment.
- typo: Add trailing period, not semicolon.
- Also, can you add language clarifying what is less accurate when
useManualCalc=true?
| @@ -209,6 +209,11 @@ describe("<TimezonePicker>", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it("manual metadata matches moment metadata", () => { | |||
There was a problem hiding this comment.
👌
Would be helpful to have a test where the two aren't equal (i.e. a "less accurate" case).
There was a problem hiding this comment.
@cmslewis had a discussions with @giladgray about possibly removing the forking logic and doing a examination about whether or not it is actually less accurate.
Specifically, I actually don't know of any case where they aren't equal but have not run a comprehensive test checking
There was a problem hiding this comment.
@cmslewis @giladgray wrote up a fiddle here to test, with { +1, 0, +1 } X untils per timezone and tested against all timezones
http://jsfiddle.net/9hnsLgx6/5/
Looks like there are 2338 unique dates, 583 timezones, 1362471 matches, 0 misses.
If this looks good to just replace the logic, I can remove the forking behavior?
Remove useManualCalc forkPreview: documentation | landing | table |
There was a problem hiding this comment.
@philcchen if you have a sec to make some style tweaks that'd be appreciated!
| const index = findOffsetIndex(timestamp, untils); | ||
| const abbreviation = getNonOffsetAbbreviation(abbrs[index]); | ||
| const offset = offsets[index] * -1; | ||
| const offsetAsString = getOffsetAsString(offset); |
There was a problem hiding this comment.
slight preference for inlining vars in object def as they're never re-used.
return {
abbreviation: getNonOffsetAbbreviation(abbrs[index]),
...
}
| const minutes = offsetVal % 60; | ||
| const hours = (offsetVal - minutes) / 60; | ||
| const sign = offset >= 0 ? "+" : "-"; | ||
| return sign + lpad(hours) + ":" + lpad(minutes); |
There was a problem hiding this comment.
slight preference for template string.
Don't use excess variables and template stringPreview: documentation | landing | table |
Checklist
Changes proposed in this pull request:
Add a prop to
<TimezonePicker>to speed up timezone calculations by skipping moment-timezone parsing of timestamps (slowww). Roughly this has the following performance benefits when turned on<TimezonePicker>, ~2x speed improvement<TimezonePicker>, ~5x speed improvementReviewers should focus on:
Naming - don't love
useManualCalc, happy to rename it to something else/whatever conforms to blueprint naming standards.Screenshot
useManualCalc: false (current/default behavior)
useManualCalc: true
Suggestions for future optimizations (not planning to do these due to lack of time 🥁 )
useManualCalc: trueby default (maybe 3.x change?). In some preliminary testing, I have yet to find a case where the manual calculation does not match the moment-timezone parsed valuegetInitialTimezoneItemsshould use the previously existing timezone items to instead of recalculating timezone items (even though it only does a subset)includeLocalTimezonefromgetInitialTimezoneItems, currentlyonComponentWillReceiveProps, if onlyshowLocalTimezonechanged, it will still re-request all timezones