-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Avoid string property access in library_webgpu.js. NFC #21454
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4690,6 +4690,7 @@ def test_webgl_simple_extensions(self, simple_enable_extensions, webgl_version): | |||||||||||||||
@parameterized({ | ||||||||||||||||
'default': ([],), | ||||||||||||||||
'closure': (['-sASSERTIONS', '--closure=1'],), | ||||||||||||||||
'closure_advanced': (['-sASSERTIONS', '--closure=1', '-O3'],), | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we need both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a few other tests that test closure with and without O3 so I was guessing there must be some expected difference: Lines 9650 to 9656 in d278e2d
Though I guess that could just be trying to change the optimization level of the Wasm. Regardless in practice it does seem to make a difference. Though as mentioned it somehow seemed nondeterministic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me to land this as is then. The nondeterministic part is certainly very worrying though. If you can share a concrete example of that I would love to to see it.. there should not be any non-determinism in the toolchain clearly. |
||||||||||||||||
'main_module': (['-sMAIN_MODULE=1'],), | ||||||||||||||||
}) | ||||||||||||||||
@requires_graphics_hardware | ||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main run that we do of closure compiler always uses
ADVANCED_OPTIMIZATIONS
IIUC.See
emscripten/tools/link.py
Line 2218 in e0bd10a
Here the closure_compiler() method is called without the
advanced
param, which meansadvanced
defaults toTrue
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed if you build with
-O0
and--closure=1
you can see (by adding -v) that it uses--compilation_level ADVANCED_OPTIMIZATIONS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how this works internally, but the output with
-O1
looks like the output withSIMPLE
and the output with-O2
looks like the output withADVANCED
. Maybe there's a more fine-grained option than SIMPLE vs ADVANCED that is causing this?The externs seem to sometimes affect both builds, but more reliably affect the ADVANCED build.