Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.14k stars 2.63k forks source link

Low: [Performance] Memoize NumberFormatUtils #42386

Open mountiny opened 1 month ago

mountiny commented 1 month ago

On hold for https://github.com/Expensify/App/issues/42200

Coming from Slack

Problem

Based on a recent profiling trace sent by David, we can observe about 33 calls to Intl.NumberFormatConstructor which collectively takes more than 400ms time on JS thread [1].

Solution

In general, the Intl should be created once and reused as much as possible to reduce the construction overhead regardless of the platform (Hermes engine discussion as an example).

The preferred solution of constructing an Intl formatter once for the entire app is not feasible as we use different formatter configurations per functionality. However, to mitigate the impact, we can memoize calls to NumberFormatUtils helpers (which are responsible for creating new formatters).

In the POC we’ve used moize library to achieve that. Library offers configuration of two critical functionalities:

cc @kacper-mikolajczak

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @johncschuster (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.

kacper-mikolajczak commented 1 month ago

Hi I am Kacper and I'd like to continue working on that! @mountiny

melvin-bot[bot] commented 1 month ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 month ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Huh... This is 4 days overdue. Who can take care of this?

johncschuster commented 1 month ago

@mountiny / @kacper-mikolajczak what's the status on this one?

mountiny commented 1 month ago

We are still discussing what standardized solution for memoization we are going to use, I think Kacker is ooo and will be back on Monday

melvin-bot[bot] commented 1 month ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 month ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

dangrous commented 1 month ago

Discussion continues

melvin-bot[bot] commented 1 month ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 4 weeks ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Eep! 4 days overdue now. Issues have feelings too...

dangrous commented 4 weeks ago

Memoize POC posted here - https://expensify.slack.com/archives/C01GTK53T8Q/p1718894753007879?thread_ts=1716371827.915779&cid=C01GTK53T8Q and on GH here - https://github.com/Expensify/App/pull/43868/files

melvin-bot[bot] commented 2 weeks ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 2 weeks ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Still overdue 6 days?! Let's take care of this!

dangrous commented 2 weeks ago

I swear I saw an update on the memoize stuff somewhere in Slack but I can't find it now. Mind giving us the latest here, @kacper-mikolajczak? Thanks!

kacper-mikolajczak commented 2 weeks ago

Hi @dangrous

The general purpose memoization tool PR is still under review here: https://github.com/Expensify/App/pull/43868

Latest Slack update: here

melvin-bot[bot] commented 1 week ago

@mananjadhav, @dangrous, @johncschuster, @kacper-mikolajczak Whoops! This issue is 2 days overdue. Let's get this updated quick!

johncschuster commented 1 week ago

Discussing here

mananjadhav commented 6 days ago

@johncschuster Can you please post the payout summary?