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.

Sign up for GitHub

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

Jump to bottom

[webgl] Donot release tensor texture at reading #6932

Merged
merged 4 commits into from
Nov 15, 2022
Merged

[webgl] Donot release tensor texture at reading #6932

merged 4 commits into from
Nov 15, 2022

Conversation

axinging
Copy link
Contributor

@axinging axinging commented Oct 13, 2022 •
edited by dsmilkov
Loading

Bug: #6920

This change is Reviewable

@axinging axinging mentioned this pull request Oct 14, 2022
Closed
@axinging axinging force-pushed the webgl_error2 branch 5 times, most recently from 15be4a4 to c92eb52 Compare October 31, 2022 03:53
@axinging axinging changed the title [webgl] Avoid release texture of shallow sliced tensor when texture has been released [webgl] Donot release tensor texture at read when tensor has been shallow sliced Oct 31, 2022
@axinging axinging changed the title [webgl] Donot release tensor texture at read when tensor has been shallow sliced [webgl] Donot release tensor texture at reading when tensor has been shallow sliced Oct 31, 2022
@axinging axinging marked this pull request as ready for review October 31, 2022 08:26
@axinging
Copy link
Contributor Author

@qjia7 @xhcao @haoyunfeix @gyagp PTAL

qjia7
qjia7 reviewed Nov 1, 2022
View reviewed changes
tfjs-backend-webgl/src/backend_webgl.ts Outdated
const {texture, dtype, texShape, usage, isPacked, slice} =
this.texData.get(dataId);
const key = slice && slice.origDataId || dataId;
const refCount = this.dataRefCount.get(key);

if (refCount > 1) {
// When reading a tensor which has been shallow sliced, do nothing.
if (slice == null && isRead) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we shouldn't call releaseGPUData for read. Otherwise, for shallowSlice tensor, we always meet problems like below case: (tensor.data() and tensor.dispose() will decrease tensor's ref count twice for the same tensor. If this tensor has a shallow sliced tensor, we will meet problem when access the shallow tensor's content since the underlying texture has been released.).

@lina128 @ping, can we just simply remove releaseGPUData there to fix this issue?

tfjs-backend-webgl/src/webgl_ops_test.ts

// Dispose b, but the texture should still remain on the GPU
// since c points to it.
b.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does your fix still work if you call await b.data(); b.dispose()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@qjia7 qjia7 requested review from pyu10055 and lina128 November 1, 2022 06:59
pyu10055
pyu10055 reviewed Nov 1, 2022
View reviewed changes
Copy link
Collaborator

@pyu10055 pyu10055 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @axinging, @lina128, and @ping)

tfjs-backend-webgl/src/backend_webgl.ts line 699 at r1 (raw file):

Previously, qjia7 (Jiajia Qin) wrote…

It seems that we shouldn't call releaseGPUData for read. Otherwise, for shallowSlice tensor, we always meet problems like below case: (tensor.data() and tensor.dispose() will decrease tensor's ref count twice for the same tensor. If this tensor has a shallow sliced tensor, we will meet problem when access the shallow tensor's content since the underlying texture has been released.).

@lina128 @ping, can we just simply remove releaseGPUData there to fix this issue?

I agree that more robust approach will be to remove releaseGPUData from
convertAndCacheOnCPU call.
@axinging can you check if that approach also make your new tests pass? thanks

@axinging
Copy link
Contributor Author

axinging commented Nov 2, 2022 •
edited
Loading

Reviewable status: 0 of 1 approvals obtained (waiting on @axinging, @lina128, and @ping)

tfjs-backend-webgl/src/backend_webgl.ts line 699 at r1 (raw file):

Previously, qjia7 (Jiajia Qin) wrote…
I agree that more robust approach will be to remove releaseGPUData from convertAndCacheOnCPU call. @axinging can you check if that approach also make your new tests pass? thanks

If simply do not release at reading, there has two test case need to be changed:
https://github.com/tensorflow/tfjs/pull/7003/files.

BTW, I tested with with all e2e models, they all get passed. so I think both solution have small impact.
@pyu10055

@axinging axinging changed the title [webgl] Donot release tensor texture at reading when tensor has been shallow sliced [webgl] Donot release tensor texture at reading Nov 7, 2022
@axinging
Copy link
Contributor Author

axinging commented Nov 7, 2022

@pyu10055 @qjia7, I change this to do not release texture at reading.PTAL

qjia7
qjia7 approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@qjia7 qjia7 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@qjia7 qjia7 requested a review from pyu10055 November 10, 2022 05:30
@axinging
Copy link
Contributor Author

@pyu10055, PTAL

pyu10055
pyu10055 approved these changes Nov 14, 2022
View reviewed changes
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @axinging, @lina128, @ping, and @qjia7)

@qjia7 qjia7 merged commit 0040b08 into tensorflow:master Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qjia7 qjia7 qjia7 approved these changes

@pyu10055 pyu10055 pyu10055 approved these changes

@lina128 lina128 Awaiting requested review from lina128

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@axinging @pyu10055 @qjia7