Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Conversation

annxingyuan
Copy link
Collaborator

@annxingyuan annxingyuan commented Jun 25, 2019

Changes

  • Add memory to backend
  • Add memory related tests from core

To see the logs from the Cloud Build CI, please join either
our discussion
or announcement mailing list.


This change is Reviewable

@annxingyuan annxingyuan requested review from dsmilkov and nsthorat June 25, 2019 16:53
@annxingyuan annxingyuan self-assigned this Jun 25, 2019
@annxingyuan annxingyuan requested a review from nkreeger June 25, 2019 16:54
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nkreeger, and @nsthorat)


tfjs-webgpu/src/backend_webgpu.ts, line 119 at r1 (raw file):

  memory(): WebGPUMemoryInfo {
    return {numBytesInGPU: this.numBytesInGPU, unreliable: false} as

you shouldn't have to cast this if you fill the fields fully

Copy link
Collaborator Author

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @nsthorat)


tfjs-webgpu/src/backend_webgpu.ts, line 119 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

you shouldn't have to cast this if you fill the fields fully

if i remove the cast it complains that the return object is missing fields from MemoryInfo like numTensors, numDataBuffers, etc. are you saying i should be filling in those fields?

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nkreeger)


tfjs-webgpu/src/backend_webgpu.ts, line 119 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

if i remove the cast it complains that the return object is missing fields from MemoryInfo like numTensors, numDataBuffers, etc. are you saying i should be filling in those fields?

Yeah can you fill those fields?

Copy link
Collaborator Author

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @nsthorat)


tfjs-webgpu/src/backend_webgpu.ts, line 119 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Yeah can you fill those fields?

the way webgl works is it only provides numBytesInGPU and unreliable, and engine merges those with its own records of numTensors / numDataBuffers. do you think webgpu should work differently?

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nkreeger)


tfjs-webgpu/src/backend_webgpu.ts, line 119 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

the way webgl works is it only provides numBytesInGPU and unreliable, and engine merges those with its own records of numTensors / numDataBuffers. do you think webgpu should work differently?

Ah apologies, we probably should have improved these typings above this (not having an extension of a base interface). LGTM for this for now.

@annxingyuan annxingyuan merged commit 0d6da52 into master Jun 26, 2019
@annxingyuan annxingyuan deleted the count_bytes_webgpu branch June 26, 2019 17:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants