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

Simplify example. Fixes #29. #30

Merged
merged 3 commits into from
Feb 18, 2020
Merged

Simplify example. Fixes #29. #30

merged 3 commits into from
Feb 18, 2020

Conversation

jakearchibald
Copy link
Contributor

No description provided.

@ricea
Copy link
Collaborator

ricea commented Feb 17, 2020

I don't know... it's certainly more concise, but it's hiding a lot of performance cost. I'm worried about sucking people into a slow pattern. I will try to do some actual measurements and get back to you.

@sindresorhus
Copy link

Show both then. With the simple one first.

@jakearchibald
Copy link
Contributor Author

If the performance is very different, maybe we need some higher level APIs:

function compressArrayBuffer(input) {
  return ReadableStream.from(input)
    .pipeThrough(new CompressionStream('deflate'))
    .arrayBuffer();
}

explainer.md Outdated Show resolved Hide resolved
@ricea
Copy link
Collaborator

ricea commented Feb 18, 2020

If the performance is very different, maybe we need some higher level APIs:

function compressArrayBuffer(input) {
  return ReadableStream.from(input)
    .pipeThrough(new CompressionStream('deflate'))
    .arrayBuffer();
}

Yes, this is a combination of whatwg/streams#1018 and whatwg/streams#1019.

@ricea
Copy link
Collaborator

ricea commented Feb 18, 2020

For compressing 50KB, the version using Blob is over 10 times slower in Chrome 80.0.3987.106:

Compressing 52101 bytes of data 10000 times with compressArrayBufferLong
compressArrayBufferLong: 810.84423828125ms
Compressing 52101 bytes of data 10000 times with compressArrayBufferShort
compressArrayBufferShort: 10981.843017578125ms

Could you retain the existing implementation under the new implementation, marked "(verbose but high-performance version)" or similar?

@jakearchibald
Copy link
Contributor Author

Whoa, any idea where the overhead comes from? The conversion to blob, or to response?

@jakearchibald
Copy link
Contributor Author

Makes me wonder if https://github.com/wicg/compression/blob/master/explainer.md#gzip-decompress-a-blob-to-a-blob is similarly slow, and makes even more of a case for the high-level methods.

@ricea
Copy link
Collaborator

ricea commented Feb 18, 2020

Whoa, any idea where the overhead comes from? The conversion to blob, or to response?

Blob, because it involves the browser process. If you use the Response constructor to convert-to-stream instead, it's only twice as slow.

@ricea
Copy link
Collaborator

ricea commented Feb 18, 2020

Makes me wonder if https://github.com/wicg/compression/blob/master/explainer.md#gzip-decompress-a-blob-to-a-blob is similarly slow, and makes even more of a case for the high-level methods.

There's an implicit assumption in this example that you're using a Blob because your data is huge. Maybe we should add an explicit note that this will perform poorly for "small" data.

@jakearchibald
Copy link
Contributor Author

Interesting! I've changed the PR to use Response rather than Blob.

@ricea
Copy link
Collaborator

ricea commented Feb 18, 2020

Thanks. I'd still like to add some guidance in how to do it performantly, but I can do that in a follow-up change.

@ricea ricea merged commit 3705d4c into whatwg:master Feb 18, 2020
@jakearchibald jakearchibald deleted the patch-2 branch February 18, 2020 10:40
@jakearchibald
Copy link
Contributor Author

Thanks for looking into the performance characteristics! I learned a lot.

@jimmywarting
Copy link

Hi 👋
I'm just curious whats make new blob([x]).stream() slower than new response(x).body 😕

@ricea
Copy link
Collaborator

ricea commented Feb 20, 2020

Hi
I'm just curious whats make new blob([x]).stream() slower than new response(x).body

See https://bugs.chromium.org/p/chromium/issues/detail?id=1053486#c1 for some background.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants