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

Indexeddb chunk store in browser #1456

Merged
merged 2 commits into from Aug 29, 2018

Conversation

@KayleePop
Copy link
Contributor

KayleePop commented Aug 2, 2018

Here's the chunk store used https://github.com/KayleePop/idbkv-chunk-store
which is built on this IndexedDB wrapper https://github.com/KayleePop/idb-kv

npm run size shows this branch's bundle to be 85,873 bytes which compared to the 85,252 bytes of master is a .7% increase or 621 bytes larger

 

Here's a deployment of instant.io using this PR's code for demo and smoke testing https://instant-io-idbkv.glitch.me/ (glitch.com is awesome)

Things to test (please share results 😄 ):

  • Very large files
  • Concurrent downloads of different torrents
  • Reload tab to load persisted data from disk
    • After download has finished
    • While download is still going
  • Multiple browser tabs (they use the same indexeddb database)
  • RAM usage
  • Speed compared to https://instant.io
    • Speed of completing download
    • downloading the file to disk
  • Streaming
  • Different browsers

EDIT: The default-chunk-store is no longer changed in this PR

@KayleePop

This comment has been minimized.

Copy link
Contributor Author

KayleePop commented Aug 2, 2018

One thing I noticed is that everything breaks in Firefox's private browsing mode (ctrl+shift+P). This is because Firefox simply disables indexeddb in that mode.

Maybe this could be fixed with another layered abstract chunk store (like immediate-chunk-store) that reverts to memory-chunk-store on failure.

@KayleePop

This comment has been minimized.

Copy link
Contributor Author

KayleePop commented Aug 2, 2018

A potential optimization is to increase or remove the concurrency limit on verification here https://github.com/webtorrent/webtorrent/blob/master/lib/torrent.js#L582 (FILESYSTEM_CONCURRENCY is set to 2)

idb-kv uses batching to improve the performance of indexeddb as each transaction adds a lot of overhead. Increasing the limit would allow more reads to make it into each batch, and it might speed up store verification.

EDIT: I added this. The benefit was so drastic that there's no reason not to. I was seeing more than a 2x speed up of verification.

EDIT2: This would load the entire torrent into memory for verification I think. The fix would be to change this from Infinity to something like a gigabyte of data at a time. (1024 * 1024 * 1024 / torrent.pieceLength)

@KayleePop KayleePop force-pushed the KayleePop:idbkv-chunk-store branch 4 times, most recently from 7958956 to 548e6c8 Aug 2, 2018
@KayleePop KayleePop force-pushed the KayleePop:idbkv-chunk-store branch from 548e6c8 to 66822bc Aug 24, 2018
@DiegoRBaquero

This comment has been minimized.

Copy link
Member

DiegoRBaquero commented Aug 26, 2018

This had been suggest before, making the default for the browser a different other than memory, but we'd rather offer extensibility than by default fill the disk and maybe never get cleaned. I'm just not convinced

@KayleePop

This comment has been minimized.

Copy link
Contributor Author

KayleePop commented Aug 26, 2018

What about just the name option and concurrency changes?

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

DiegoRBaquero commented Aug 26, 2018

Sure, for now, and we can keep the "debate" open

@KayleePop KayleePop force-pushed the KayleePop:idbkv-chunk-store branch from 66822bc to c93d2d2 Aug 26, 2018
@KayleePop

This comment has been minimized.

Copy link
Contributor Author

KayleePop commented Aug 26, 2018

I removed the commit that changed the default store.

KayleePop added 2 commits Aug 26, 2018
IndexedDB chunk stores benefit from maximum concurrency
@KayleePop KayleePop force-pushed the KayleePop:idbkv-chunk-store branch from c93d2d2 to 1479c3a Aug 27, 2018
Copy link
Member

DiegoRBaquero left a comment

LGTM

@DiegoRBaquero DiegoRBaquero merged commit 06379b5 into webtorrent:master Aug 29, 2018
3 checks passed
3 checks passed
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@KayleePop KayleePop deleted the KayleePop:idbkv-chunk-store branch Aug 30, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.