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

Encoding: use Wasm to get a SharedArrayBuffer instance #22361

Merged
merged 4 commits into from Mar 24, 2020

Conversation

@annevk
Copy link
Member

@annevk annevk commented Mar 20, 2020

For #22358.

@wpt-pr-bot wpt-pr-bot added the encoding label Mar 20, 2020
@wpt-pr-bot wpt-pr-bot requested a review from inexorabletash Mar 20, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22361 Mar 20, 2020 Inactive
return new ArrayBuffer(length);
} else {
// See https://github.com/whatwg/html/issues/5380 for why not `new SharedArrayBuffer()`
// WebAssembly.Memory's size is in multiples of 64 KiB

This comment has been minimized.

@ricea

ricea Mar 23, 2020
Contributor

Does this mean we actually create a 64 KiB buffer here?

Could we instead grab a copy of buffer's constructor and use that to construct a SharedArrayBuffer instance of the exact size specified?

This comment has been minimized.

@annevk

annevk Mar 23, 2020
Author Member

Oh, that's cunning. Yeah, I think that would work.

cc @syg @domenic

This comment has been minimized.

@annevk

annevk Mar 23, 2020
Author Member

Done.

@annevk annevk force-pushed the annevk/encoding-wasm-sab branch from 9774d70 to 86d52aa Mar 23, 2020
@annevk annevk force-pushed the annevk/encoding-wasm-sab branch from 86d52aa to 0dff2c4 Mar 23, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22361 Mar 23, 2020 Inactive

const emptyChunk = new Uint8Array(new self[arrayBufferOrSharedArrayBuffer](0));
const inputChunk = new Uint8Array(new self[arrayBufferOrSharedArrayBuffer](inputChunkData.length));
function createBuffer(type, length = 0) {

This comment has been minimized.

@ricea

ricea Mar 23, 2020
Contributor

Does this need to be inside the loop?

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22361 Mar 23, 2020 Inactive
@ricea
Copy link
Contributor

@ricea ricea commented Mar 23, 2020

Non-owner lgtm.

@annevk
Copy link
Member Author

@annevk annevk commented Mar 23, 2020

@ricea I think you should feel free to call yourself an owner of this directory.

Copy link
Member

@domenic domenic left a comment

LGTM with potential improvements.

@@ -1,3 +1,13 @@
function createBuffer(type, length = 0) {

This comment has been minimized.

@domenic

domenic Mar 23, 2020
Member

It may be worth pulling this out into /common

This comment has been minimized.

@annevk

annevk Mar 23, 2020
Author Member

I was thinking to maybe do that, okay. And then use some closure to only do the WebAssembly thing once and hide the constructor. /common/sab.js okay? I'll prolly clean up these 4 PRs with that tomorrow then.

This comment has been minimized.

function createBuffer(type, length = 0) {
if (type === "ArrayBuffer") {
return new ArrayBuffer(length);
} else {

This comment has been minimized.

@domenic

domenic Mar 23, 2020
Member

Since this is a test, IMO it's worth doing an explicit test that type === "SharedArrayBuffer", and throwing an exception if it's neither of the two. (A switch/case would also work.)

@wpt-pr-bot wpt-pr-bot added the assets label Mar 24, 2020
@wpt-pr-bot wpt-pr-bot requested review from deniak and zqzhang Mar 24, 2020
@annevk
Copy link
Member Author

@annevk annevk commented Mar 24, 2020

I created sab.js. I guess it would be good for this to land first and then we see if we want to use it in the other PRs. (Edit: they don't need it. They are just creating a single instance and I don't really want to complicate them all by loading another file.)

@annevk annevk merged commit 4e83bff into master Mar 24, 2020
11 checks passed
11 checks passed
detect-deployment
Details
Azure Pipelines Build #20200324.21 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@annevk annevk deleted the annevk/encoding-wasm-sab branch Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants