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

Avoid string property access in library_webgpu.js. NFC #21454

Merged
merged 4 commits into from
Mar 8, 2024
Merged

Avoid string property access in library_webgpu.js. NFC #21454

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f50dc80
Avoid string property access in library_webgpu.js. NFC
sbc100 Feb 29, 2024
23f15f1
add closure+O3 mode to test_webgpu_basic_rendering
kainino0x Mar 7, 2024
efeb199
Merge branch 'main' into webgpu_props
kainino0x Mar 7, 2024
354114c
Merge branch 'main' into webgpu_props
sbc100 Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions test/test_browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4690,6 +4690,7 @@ def test_webgl_simple_extensions(self, simple_enable_extensions, webgl_version):
@parameterized({
'default': ([],),
'closure': (['-sASSERTIONS', '--closure=1'],),
Copy link
Collaborator Author

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

final_js = building.closure_compiler(final_js, extra_closure_args=options.closure_args)
.

Here the closure_compiler() method is called without the advanced param, which means advanced defaults to True.

Copy link
Collaborator Author

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

Copy link
Collaborator

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 with SIMPLE and the output with -O2 looks like the output with ADVANCED. 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.

'closure_advanced': (['-sASSERTIONS', '--closure=1', '-O3'],),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure we need both -O0 and -O3 here.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

emscripten/test/test_other.py

Lines 9650 to 9656 in d278e2d

@parameterized({
'': ([],), # noqa
'O3': (['-O3'],), # noqa
'closure': (['--closure=1'],), # noqa
'closure_O3': (['--closure=1', '-O3'],), # noqa
})
def test_EM_ASM_ES6(self, args):

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down