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

Replace var with const/let in library_(html5_)webgpu.js #21488

Closed
wants to merge 1 commit into from

Conversation

kainino0x
Copy link
Collaborator

@kainino0x kainino0x commented Mar 7, 2024

Needs rebase after #21454

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

The funny thing about using these newer constructs is that they can come with downsides:

  1. At runtime IIUC var can be a little faster since let and const involve tracking some kind of dead zone information.
  2. They can make the JS larger because const is a longer string that var
  3. They prohibit closure from re-writeing all var declaration into a single one per function (since let/const inside blocks are not supposed to be visible outside block).

None of these are a huge deal.. but its why we haven't made efforts to convert over yet.

CC @syg

@kainino0x
Copy link
Collaborator Author

kainino0x commented Mar 8, 2024

interesting, I had guessed that Closure's analysis would take care of problems like that. A transform must be required for targeting ES5 so I kind of assumed that they would just do it unconditionally, instead of going through the work of preserving let/const when the target is ES6+. Since they do variable renaming, they already know what all the variables resolve to so I'd think they can do it without causing any observable behavior difference.

This comment does seem to agree with my suspicion, although it's 3.5 years old. google/closure-compiler#3680 (comment)

@kainino0x kainino0x marked this pull request as ready for review March 8, 2024 23:28
@sbc100
Copy link
Collaborator

sbc100 commented Mar 8, 2024

interesting, I had guessed that Closure's analysis would take care of problems like that. A transform must be required for targeting ES5 so I kind of assumed that they would just do it unconditionally, instead of going through the work of preserving let/const when the target is ES6+. Since they do variable renaming, they already know what all the variables resolve to so I'd think they can do it without causing any observable behavior difference.

This comment does seem to agree with my suspicion, although it's 3.5 years old. google/closure-compiler#3680 (comment)

It was my assumption that closure compiler would indeed do this, but last time I tried to do var -> let conversions they came with a code size code because of the lack of coalescing. Might be worth further investigation?

@kainino0x
Copy link
Collaborator Author

Sounds like it's time to finally add a WebGPU code size test. I'll try to look into that next week.

@kainino0x
Copy link
Collaborator Author

Closing for now, filed as a bug in #22496.

@kainino0x kainino0x closed this Sep 3, 2024
@kainino0x kainino0x deleted the webgpu-warnings branch September 3, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants