Skip to content

Conversation

@chunnienc
Copy link
Collaborator

@chunnienc chunnienc commented Feb 16, 2023

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


This change is Reviewable

@chunnienc chunnienc marked this pull request as ready for review February 17, 2023 18:14
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 with a nit.

Comment on lines 30 to 31
private freeTextures?: Record<string, Texture[]> = {};
private usedTextures?: Record<string, Texture[]> = {};
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Why is this optional (with ?) but immediately assigned? Does it get deleted somewhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. TextureManager.dispose() assigns nulls to them, and a lot of tests relies on this behavior (set to null after disposed) to be correct, at least they failed when I tried to make it always not null. There may be some additional guards/checks needed after we turn on the null check, but so far it's fine since it matches the old behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

TextureManager.dispose() means TextureManager's singleton is disposed and thus its freeTextures and usedTextures could be disposed, which does not require freeTextures and usedTextures to be optional.

On the other hand, if freeTextures and usedTextures are optional, whenever they are used, null check is required, causing the runtime to execute more logics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rolled back and added a todo comment

@chunnienc chunnienc merged commit 0e3d667 into tensorflow:master Feb 22, 2023
@chunnienc chunnienc deleted the webgl-perf branch February 22, 2023 05:52
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.

3 participants