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.

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

Add write queue to localForage storage provider #121

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Mar 8, 2022

cc @kidroca curious if you see any issues with this approach

Details

IndexedDB writes are causing major page jank when refreshing a page or signing in for the first time.

Trying to figure out a solution in this PR hopefully.

Just added a basic kind of queue that will limit the total number of parallel writes to IDB so we aren't locking things up too bad.

Definitely got something wrong here as there's an issue on first sign in where no data seems to load until the page is refreshed. Still looking into why, but I think in theory if we just slow down writing to IndexedDB it should help performance. Has went away and seems unrelated to the changes.

Related Issues

#119

Automated Tests

Modified existing test

Linked PRs

Expensify/App#8053

@marcaaron marcaaron self-assigned this Mar 8, 2022
Copy link
Contributor

@kidroca kidroca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might alleviate the writing problem (notice any difference?), but it still creates parallel writes

I think the source of the problem as long as we're using localForage is parallel writes happening

localForage stores all our key/values under a single object store and creates a readwrite transaction for each setItem call: https://github.com/localForage/localForage/blob/711d5891dfc699705f086c2bec4c68d6c363c8ff/src/drivers/indexeddb.js#L648-L651

When readonly transactions using the same object store overlap - it's not a problem
When readwrite transaction using the same object store overlap - they have to wait each other

https://www.w3.org/TR/IndexedDB/#transaction-scheduling

If multiple read/write transactions are attempting to access the same object store (i.e. if they have overlapping scopes), the transaction that was created first is the transaction which gets access to the object store first, and it is the only transaction which has access to the object store until the transaction is finished.

  1. If the Browser is already queueing the transactions for us, having our own queue would probably not help much
  2. Since parallel writes are not possible, we can just queue writes to happen sequentially one at a time

IMO if this PR is noticeably improving performance we can use it while we work on a new storage provider
Using idb we can take control of readwrite transactions and write multiple keys in a single transaction

@kidroca
Copy link
Contributor

kidroca commented Mar 8, 2022

BTW we can try a mix of idb and localForage - our problem is with writes we can implement setItem to batch and then write data in one go using idb, while we still use localForage for everything else (reading)
Once that's working great, we can work on dropping localForage

@marcaaron
Copy link
Contributor Author

If the Browser is already queueing the transactions for us, having our own queue would probably not help much
Since parallel writes are not possible, we can just queue writes to happen sequentially one at a time

Ah that's a solid point then. So there's maybe no reason to "batch" anything since the writes can only happen 1 at a time anyway. The queueing must be mainly giving the JS a chance to breathe in between the blocking sequential writes then.

BTW we can try a mix of idb and localForage - our problem is with writes we can implement setItem to batch and then write data in one go using idb, while we still use localForage for everything else (reading)

That sounds like a great plan.

@parasharrajat
Copy link
Member

parasharrajat commented Mar 8, 2022

Using idb we can take control of readwrite transactions and write multiple keys in a single transaction

👍 🏆 . This will be eventually needed.

@marcaaron
Copy link
Contributor Author

Changes are simplified now. This greatly improves the first load performance for me on web/desktop and seems like everything works normally otherwise.

@marcaaron marcaaron marked this pull request as ready for review March 8, 2022 20:00
@marcaaron marcaaron requested a review from a team as a code owner March 8, 2022 20:00
@MelvinBot MelvinBot requested review from amyevans and removed request for a team March 8, 2022 20:01
@marcaaron
Copy link
Contributor Author

cc @Julesssss @AndrewGable this could use another set of 👀 from some past contributors :D

Copy link
Contributor

@AndrewGable AndrewGable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! I'd be interested in seeing the idb solution as well

@kidroca
Copy link
Contributor

kidroca commented Mar 9, 2022

Submitted a proposal about it here: Expensify/App#7950 (comment)

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to catch up with localForage and Kidroca's points, but this looks good 👍

Also interested to see what an idb improvement looks like!

@marcaaron marcaaron merged commit 032ff54 into main Mar 9, 2022
@marcaaron marcaaron deleted the marcaaron-slowDownLocalForageWrite branch March 9, 2022 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants