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

fix: move blurhash worker operations to before status rendering #1391

Merged
merged 4 commits into from Aug 17, 2019

Conversation

nolanlawson
Copy link
Owner

As described in #1381 (comment) I would like to avoid the "pop" where the blurhash suddenly shows after the worker is done decoding it. This moves blurhash decoding into the same point in time when we asynchronously fetch status data from IndexedDB or the cache, so it happens before status rendering.

Other changes:

  • Use promise-worker because I wrote it and I like it :)
  • Add some perf marks/measures so I can get a better idea of the timeline
  • Move init() of worker a bit earlier

I'm still dissatisfied with the worker round-trip time (on my local Chrome desktop it seems to take like a few hundred milliseconds... is that normal? is it just a dev tools artifact?). Also I'd like to move the cache away from the worker if we can't fix that round-trip time.

init()
// TODO: should maintain a cache outside of worker to avoid round-trip for cached data
const { encoded, decoded, imageData } = await worker.postMessage(blurhash)
if (encoded !== blurhash) { // TODO: why do we check this? Shouldn't it always be the same?
Copy link
Owner Author

Choose a reason for hiding this comment

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

@sorin-davidoi I didn't catch this in the last PR, but do you know why we need to pass the encoded string into the worker, have the worker return it, and then check that it's exactly the same as the value we already gave it? Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because we are listening for all messages posted by the Web Worker, so we might also get decodings meant for other media.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ahhh, understood. I think now that we're using promise-worker, we should be safe. It keeps track of all messages using an internal autoincrementing ID, so we won't get the wrong message (as long as all worker interactions are routed through promise-worker).

@nolanlawson
Copy link
Owner Author

Huh, I'm looking at Chrome tracing, and it's really unclear to me why there's such a long delay waiting for the worker to return the message. It looks like just pure idle time. Something about RunBestEffortPriorityTask which apparently takes a long time to fire?

@nolanlawson
Copy link
Owner Author

Stopped sending the encoded data back and forth from the worker, and moved the cache outside of the worker to avoid round-trips.

@nolanlawson
Copy link
Owner Author

Hm, it seems that the OffscreenCanvas approach is just way slower in Chrome. I'm getting ~600ms for the total time to decode a blurhash, as opposed to ~15ms in Firefox. When I switch Chrome to not use OffscreenCanvas, it starts taking 20-100ms in Chrome.

Can't tell if the problem is OffscreenCanvas, or creating a blob URL in a worker, or what.

@nolanlawson
Copy link
Owner Author

So it turns out that offscreenCanvas.convertToBlob() is very slow in Chrome, at least on Linux. Takes up to 800ms when running multiple in parallel at least.

@nolanlawson
Copy link
Owner Author

Odd, I tried creating a new OffscreenCanvas every time instead of reusing the same one, but the perf doesn't change. 800ms or so. Maybe this is just a Linux thing; I need to test more.

@nolanlawson nolanlawson merged commit f8180e8 into master Aug 17, 2019
@nolanlawson
Copy link
Owner Author

Hm, OffscreenCanvas is slow on Chrome for Android as well - between 700ms and 1.2s. Not sure if we can use this API right now

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.

None yet

2 participants