fix(offline): Speed up offline storage by ~87% #4176
Conversation
joeyparrish
added
type: performance
| @@ -441,36 +441,34 @@ shaka.offline.Storage = class { | |||
| async downloadSegments_( | |||
| toDownload, manifestId, manifestDB, downloader, config, storage, | |||
| manifest, drmEngine) { | |||
| let pendingManifestUpdates = {}; | |||
There was a problem hiding this comment.
What happens to these pending manifest updates if we throw due to tripping the this.ensureNotDestroyed_()? If that happens, they might not be added to the manifest, so the static cleanup code won't know to clean them up.
There was a problem hiding this comment.
I think that what would happen now is:
- Downloaded segments are stored in the database
- Those segments are orphaned and not referred to in any manifest
That can also happen if the user closes the page, though.
Let me look at two things in a follow-up:
- How to deal with orphaned segments (which can happen without cancellation)
- How to clean up segments in the same session on abort()
| @@ -538,19 +554,18 @@ shaka.offline.Storage = class { | |||
| * changes of the other. | |||
| * | |||
| * @param {number} manifestId | |||
| * @param {!shaka.media.SegmentReference} ref | |||
| * @param {shaka.extern.SegmentDataDB} data | |||
| * @param {!Object.<string, number>} manifestUpdates | |||
There was a problem hiding this comment.
The JSDoc description for this function is now out of date. It should now talk about manifestUpdates instead of ref, for instance.
lib/offline/storage.js
Outdated
| const dataKeys = await activeHandle.cell.addSegments([data]); | ||
| dataKey = dataKeys[0]; | ||
| throwIfAbortedFn(); | ||
|
|
||
| // Acquire the mutex before accessing the manifest, since there could be | ||
| // multiple instances of this method running at once. | ||
| mutexId = await shaka.offline.Storage.mutex_.acquire(); |
There was a problem hiding this comment.
I don't think we need this mutex anymore? Since any given store operation is only going to be running this method once or twice anyway, and in the two-calls case they definitely don't happen at the same time. So there's no risk of a given storage entry being mutated by two different threads at once.
And if we get rid of this mutex, we could get rid of the shaka.util.Mutex class too.
lib/offline/storage.js
Outdated
| * @param {function()} throwIfAbortedFn A function that should throw if the | ||
| * download has been aborted. | ||
| * @return {!Promise.<?shaka.extern.ManifestDB>} | ||
| */ | ||
| static async assignStreamToManifest(manifestId, ref, data, throwIfAbortedFn) { | ||
| static async assignStreamToManifest( |
There was a problem hiding this comment.
Nit: The method should probably be called assignStreamsToManifest now that it can do multiple at once.
There was a problem hiding this comment.
Renamed to assignSegmentsToManifest, since it is now only setting the database keys of the segments. The streams are already part of the manifest.
- removed Mutex - updated method docs - renamed assignStreamToManifest to assignSegmentsToManifest
|
Bah, linter failed! Fixed now, but sadly, I'll need another approval. |
If a storage operation is aborted (via API, not via closing the page), this will now be cleaned up from the database. More work is needed to find and remove orphaned segments in the database. Related to shaka-project#4166 and follow-up to PR shaka-project#4176.
By waiting for all segment data to be written to the database before updating the manifest, we can speed up offline storage in the foreground by ~87%. Closes #4166
github-actions
bot
added
the
status: archived
By waiting for all segment data to be written to the database before updating the manifest, we can speed up offline storage in the foreground by ~87%.
Closes #4166