[HOLD for payment 2024-09-23] Performance Improvement: Update formatjs libraries and upstream/patch date-fns-tz

[hurali97]

Problem

While we were analysing the latest traces from Jason, we realised that there's substantial amount of time spent in formatjs and date-fns-tz. We also double-checked this with our heavy accounts (~15k reports) and we got the same results. Following is how things look like on baseline:

Baseline

snapshot from profile

Solution:

While analysing the traces, we curated a list of places that can be improved in formatjs and date-fns-tz. Two noticeable items are BestFitMatcher from formatjs and getDateTimeFormat from date-fns-tz. Now, since these are third party libraries, we can bump them to the latest versions and see if they are already fixing performance related issues. We compared the code for BestFitMatcher from the currently installed version formatjs/intl-locale@3.3.0 with formatjs/intl-locale@4.0.0 and there was a significant refactor of this file.

So it only made sense to bump formatjs libraries to the latest to get the performance improvements out of the box. For date-fns-tz there was no significant updates to getDateTimeFormat function, so to apply the improvements we have two options:

• Patch date-fns-tz
• Upstream date-fns-tz by creating a PR to it's repository

However, we don't know how long it would take for option 2 as it depends on the reviewer of that repo. We can maybe move forward by mixing both options and removing the patch, once our PR is merged and available on NPM.

The improvement for getDateTImeFormat is pretty simple. We have Intl.DateTimeFormat being re-initialised every time getDateTimeFormat is invoked. Since testDateFormatted is a static variable, we can move it outside of the function.

var dtfCache = {};

// Moved this out from `getDateTimeFormat`
    var testDateFormatted = new Intl.DateTimeFormat('en-US', {
      hour12: false,
      timeZone: 'America/New_York',
      year: 'numeric',
      month: 'numeric',
      day: '2-digit',
      hour: '2-digit',
      minute: '2-digit',
      second: '2-digit'
    }).format(new Date('2014-06-25T04:00:00.123Z'));
    var hourCycleSupported = testDateFormatted === '06/25/2014, 00:00:00' || testDateFormatted === '‎06‎/‎25‎/‎2014‎ ‎00‎:‎00‎:‎00';
    

function getDateTimeFormat(timeZone) {
 ...someImplementation
  return dtfCache[timeZone];
}

Below is how things look like after formatjs version bumps and date-fns-tz patch:

snapshot from profile

---

To summarise;

• Bump formatjs libraries to the latest
• Patch or Upstream date-fns-tz with the changes described above

---

[melvin-bot]

Current assignee @mountiny is eligible for the AutoAssignerNewDotQuality assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[roryabraham]

we can maybe move forward by mixing both options and removing the patch, once our PR is merged and available on NPM

This is generally my preferred approach. Report the issue with a minimal reproduction, and create a PR to fix it upstream. If you don't receive any feedback, proceed with a patch and create an issue to eventually remove the patch once the upstream fix is merged. If you do get feedback and the maintainer doesn't want to merge your fix for some reason, re-evaluate and open up a conversation with the team.

[roryabraham]

looks like some good finds @hurali97 👍🏼

[hurali97]

PR for date-fns-tz library is here. Now, we can wait for two days and if there's no update from the reviewer, we will go ahead with creating the patch in Expensify.

[roryabraham]

sounds great 👍🏼

[Ollyws]

Could someone process payment for my review of #48067, thanks!

[mountiny]

@Ollyws sorry, the automation failed here.

$250 to @Ollyws for their review, no regression tests required for this task cc @strepanier03

[mountiny]

Noting we do not want to close this but make it monthly so we can track of the upstream PR

[Ollyws]

Requested in ND.

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[garrettmknight]

$250 approved for @Ollyws

[mountiny]

I think we can close now then