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

SharedArrayBuffer is not GPU data #7344

Merged
merged 5 commits into from
Feb 10, 2023

Conversation

axinging
Copy link
Contributor

@axinging axinging commented Feb 6, 2023

Bug: #7343

TODO: Add test case for ShardArrayBuffer.

describeWithFlags('tensor from ShardArrayBuffer', ALL_ENVS, () => {
  it('float32 tensor from ShardArrayBuffer', async () => {
    let length = 4;
    // ReferenceError: SharedArrayBuffer is not defined.
    let sharedFloats = new Float32Array(new SharedArrayBuffer(length * 4));
    let expected = [];
    for (let i = 0; i < length; i++) {
      sharedFloats[i] = i;
      expected[i] = i;
    }
    const a = tf.tensor(sharedFloats);
    tf.test_util.expectArraysEqual(await a.data(), expected);
  });
});


This change is Reviewable

Comment on lines 212 to 217
if ('texture' in values) {
return GPUDataType.WebGL;
} else if (
'buffer' in values && !(values.buffer instanceof ArrayBuffer) &&
!(values.buffer instanceof SharedArrayBuffer)) {
return GPUDataType.WebGPU;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great catch, thanks you!

I am considering, rather than excluding cases, what about we do checks like:?

if ('texture' in values && values.texture instanceof WebGLTexture) {
  return GPUDataType.WebGL;
} else if ( 'buffer' in values && values.buffer instanceof GPUBuffer) {
...
}

Copy link
Contributor Author

@axinging axinging Feb 7, 2023

Choose a reason for hiding this comment

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

@Linchenn, if use 'buffer' in values && values.buffer instanceof GPUBuffer, some bots targets (cpu, wasm...) will complain: ReferenceError: GPUBuffer is not defined.

BTW, I am trying to add a test case 'tensor from ShardArrayBuffer', however, chrome complains 'ReferenceError: SharedArrayBuffer is not defined.' It seems 'yarn karma start' doesn't support SharedArrayBuffer . This can be workaround by https://github.com/Honry/webnn-samples/blob/tflite-deeplab/server.js.

@qjia7 @xhcao @haoyunfeix @gyagp , PTAL

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that 'buffer' in values is hard to distinguish WebGPUData from other objects with buffer key.

Could we rename WebGPUData's buffer filed to something else? but it might have to change lots of codes. I am just providing a candidate idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about this idiom (though I don't feel parenthesis is needed around the operand of typeof)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @hujiajie, it seems the bots turrn green.

@Linchenn , Could you please take another look?

@axinging axinging marked this pull request as ready for review February 7, 2023 03:06
Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

Nice workaround. LGTM, thanks!

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

Sorry for the late review!

tfjs-core/src/types.ts Outdated Show resolved Hide resolved
tfjs-core/src/tensor_util_env.ts Outdated Show resolved Hide resolved
tfjs-core/src/tensor_util_env.ts Outdated Show resolved Hide resolved
tfjs-core/src/ops/tensor_ops_util.ts Outdated Show resolved Hide resolved
tfjs-core/src/ops/tensor_ops_util.ts Outdated Show resolved Hide resolved
@axinging axinging marked this pull request as draft February 9, 2023 07:05
Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

LGTM

@axinging axinging marked this pull request as ready for review February 10, 2023 00:21
@Linchenn Linchenn added this pull request to the merge queue Feb 10, 2023
@mattsoulanille mattsoulanille removed this pull request from the merge queue due to the queue being cleared Feb 10, 2023
@mattsoulanille mattsoulanille merged commit c4160b7 into tensorflow:master Feb 10, 2023
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.

4 participants