Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Performance] localforage.setItem() is causing blocking behavior on init for web clients #7950

Closed
mallenexpensify opened this issue Mar 1, 2022 · 21 comments
Closed

[Performance] localforage.setItem() is causing blocking behavior on init for web clients #7950

mallenexpensify opened this issue Mar 1, 2022 · 21 comments
Assignees
@marcaaron
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@mallenexpensify
Copy link
Contributor

Creating this in E/App repo from this issue in the E/react-native-onyx repo, in case an external contributor can work on it.

Problem

When the app inits and many keys need to be set things get really slow on web. It currently takes me about 10-12 seconds to switch to a new chat when the app inits.

We've narrowed down the source of the problem to:

  1. Not stringifying IndexedDB values. For whatever reason IDB takes a while to store structured JSON.
  2. Calling setItem() too many times in a row

Addressing either one should work. Both seem to be causing similar rates of pain.

Solution

  1. Start stringifying values - complicated because some of the JSON is not serializable i.e. stuff with attachments.
  2. Defer the writing to IndexedDB until we are idle or just slow it down or something at least. We are using a memory cache already so I can't think of a reason why our writes must happen immediately on page load.

More ideas here: https://rxdb.info/slow-indexeddb.html, but the general suggestion is to use fewer transactions. That is not possible with localforage so we might want to look elsewhere - maybe the plugin mentioned here can help localForage/localForage#315

I did a test here where I just made all the writes synchronous (probably not the solution we want), but it improves things immensely

Expensify/react-native-onyx#118

cc @marcaaron
@mallenexpensify mallenexpensify added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Mar 1, 2022
@MelvinBot
Copy link

Triggered auto assignment to @CortneyOfstad (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 1, 2022
@CortneyOfstad CortneyOfstad removed their assignment Mar 1, 2022
@CortneyOfstad CortneyOfstad added Improvement Item broken or needs improvement. Engineering labels Mar 1, 2022
@MelvinBot
Copy link

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Mar 1, 2022
@MelvinBot
Copy link

Triggered auto assignment to @dylanexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@mallenexpensify
Copy link
Contributor Author

@roryabraham dropped a comment in #expensify-open-source and tagged @kidroca
https://expensify.slack.com/archives/C01GTK53T8Q/p1646173137404599

@dylanexpensify
Copy link
Contributor

@mallenexpensify am I still good to export this to Upwork?

@mallenexpensify mallenexpensify changed the title [Performance] localforage.setItem() is causing blocking behavior on init for web clients [$1000] [Performance] localforage.setItem() is causing blocking behavior on init for web clients Mar 2, 2022
@mallenexpensify
Copy link
Contributor Author

Please do @dylanexpensify , lets start tis one at $1k, I have a feeling it's a significant issue and it's 'performance' so we'd like to get 👀 on it sooner than later

@mallenexpensify
Copy link
Contributor Author

Possibly related, I'd like to try to test after this issue is fixed to see if can reproduce

@dylanexpensify , I posted the job since I was back in here...

https://www.upwork.com/jobs/~01d8bffdd4e6f23ff8

@botify botify removed the Daily KSv2 label Mar 2, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Mar 2, 2022
@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 2, 2022
@MelvinBot
Copy link

Current assignee @thienlnam is eligible for the Exported assigner, not assigning anyone new.

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 2, 2022
@MelvinBot
Copy link

📣 @rushatgabhane You have been assigned to this job by @mallenexpensify!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@mallenexpensify
Copy link
Contributor Author

Reassigning C+ to @rushatgabhane , @Santhosh-Sellavel is starting this/next week
@kidroca , can you please look into? For a fix or to help with a fix

@dylanexpensify dylanexpensify removed their assignment Mar 11, 2022
@dylanexpensify dylanexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Mar 11, 2022
@MelvinBot
Copy link

Triggered auto assignment to @laurenreidexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Mar 11, 2022
@dylanexpensify
Copy link
Contributor

Reassigning as I'm OOO next week! Thanks Lauren! I'll pick this up when I'm back!

@dylanexpensify dylanexpensify self-assigned this Mar 11, 2022
@dylanexpensify dylanexpensify added Weekly KSv2 and removed Daily KSv2 labels Mar 11, 2022
@thienlnam
Copy link
Contributor

thienlnam commented Mar 11, 2022 •
edited
Loading

It might turn out that the SyncQueue implemented in @marcaaron PR is completely satisfying performance and there's no need for all of this

Going to go ahead and close this then until we run into performance issues again, and can revisit this later

@marcaaron
Copy link
Contributor

Linked PRs to fix this are not merged yet so gonna reopen this and assign myself.

@marcaaron marcaaron reopened this Mar 14, 2022
@marcaaron marcaaron added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Mar 14, 2022
@mallenexpensify mallenexpensify changed the title [$1000] [Performance] localforage.setItem() is causing blocking behavior on init for web clients [Performance] localforage.setItem() is causing blocking behavior on init for web clients Mar 15, 2022
@marcaaron marcaaron mentioned this issue Mar 17, 2022
Closed
@marcaaron
Copy link
Contributor

a fix should be on staging soon.

@MelvinBot MelvinBot removed the Overdue label Mar 23, 2022
@marcaaron
Copy link
Contributor

Seems like changes are on production now gonna close this!

@MelvinBot MelvinBot removed the Overdue label Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@marcaaron marcaaron

Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests