Skip to content

Simplify _scriptName / scriptDirectory handling. NFC #24023

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
Merged
merged 5 commits into from
Apr 17, 2025
Merged

Simplify _scriptName / scriptDirectory handling. NFC #24023

merged 5 commits into from
Apr 17, 2025

Conversation

kleisauke
Copy link
Collaborator

@kleisauke kleisauke commented Apr 1, 2025 •
edited
Loading

  • Avoid defining _scriptName outside the wrapper function in EXPORT_ES6 mode and for Node.js CommonJS builds.
  • Simplify scriptDirectory resolution to new URL('.', _scriptName).href on the web.
  • Add tests to verify that scriptDirectory is an absolute path across different build configurations.

@kleisauke kleisauke mentioned this pull request Apr 1, 2025
Closed
@sbc100
Copy link
Collaborator

sbc100 commented Apr 1, 2025

Can you add some info to the PR description.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 1, 2025

Presumably this comes with some code size deltas?

sbc100
sbc100 reviewed Apr 1, 2025
View reviewed changes
@kleisauke
Copy link
Collaborator Author

Marking as draft until I've written a PR description and rebasedlined the test expectations.

@kleisauke kleisauke marked this pull request as draft April 1, 2025 18:14
@kleisauke kleisauke force-pushed the simplify-script-dir branch 3 times, most recently from 944aaf2 to 89d55ff Compare April 2, 2025 17:49
@kleisauke kleisauke force-pushed the simplify-script-dir branch 2 times, most recently from 76864c8 to c457e42 Compare April 16, 2025 08:53
@kleisauke
Copy link
Collaborator Author

Rebasedlined the test expectations and added a (short) PR description summarizing the changes. The changes to shell_minimal.js and wasm_worker.js are NFC and could probably be split into a separate PR.

This is ready for review now.

@kleisauke kleisauke marked this pull request as ready for review April 16, 2025 09:03
sbc100
sbc100 reviewed Apr 16, 2025
View reviewed changes
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.

Great!

Out of interest, do any/all of the new tests fail without this change?

@kleisauke
Simplify _scriptName / scriptDirectory handling. NFC
2d867d1 
- Avoid defining `_scriptName` outside the wrapper function in
  `EXPORT_ES6` mode and for Node.js CommonJS builds.
- Simplify `scriptDirectory` resolution to
  `new URL('.', _scriptName).href` on the web.
@kleisauke
Incorporate review comments
a2d8b09
@kleisauke
Copy link
Collaborator Author

Out of interest, do any/all of the new tests fail without this change?

The new tests also seem to pass on the main branch. IIRC, the other.test_locate_file_abspath_pthread test used to fail, but it was likely fixed when the pthread *.worker.js file was inlined into the main output file.

Given this, this PR is (likely) a non-functional change now. I'll add the NFC keyword to the title.

@kleisauke
Automatic rebaseline of codesize expectations. NFC
65b2a41 
This is an automatic change generated by tools/maint/rebaseline_tests.py.

The following (86) test expectation files were updated by
running the tests with `--rebaseline`:

```
browser/test_small_js_flags.js.size: 3996 => 3934 [-62 bytes / -1.55%]
other/codesize/test_codesize_cxx_ctors1.gzsize: 8237 => 8208 [-29 bytes / -0.35%]
other/codesize/test_codesize_cxx_ctors1.jssize: 19928 => 19920 [-8 bytes / -0.04%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8226 => 8196 [-30 bytes / -0.36%]
other/codesize/test_codesize_cxx_ctors2.jssize: 19906 => 19898 [-8 bytes / -0.04%]
other/codesize/test_codesize_cxx_except.gzsize: 9227 => 9203 [-24 bytes / -0.26%]
other/codesize/test_codesize_cxx_except.jssize: 23665 => 23657 [-8 bytes / -0.03%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8178 => 8156 [-22 bytes / -0.27%]
other/codesize/test_codesize_cxx_except_wasm.jssize: 19821 => 19813 [-8 bytes / -0.04%]
other/codesize/test_codesize_cxx_except_wasm_legacy.gzsize: 8178 => 8156 [-22 bytes / -0.27%]
other/codesize/test_codesize_cxx_except_wasm_legacy.jssize: 19821 => 19813 [-8 bytes / -0.04%]
other/codesize/test_codesize_cxx_lto.gzsize: 8244 => 8217 [-27 bytes / -0.33%]
other/codesize/test_codesize_cxx_lto.jssize: 20003 => 19994 [-9 bytes / -0.04%]
other/codesize/test_codesize_cxx_mangle.gzsize: 9270 => 9246 [-24 bytes / -0.26%]
other/codesize/test_codesize_cxx_mangle.jssize: 23780 => 23772 [-8 bytes / -0.03%]
other/codesize/test_codesize_cxx_noexcept.gzsize: 8237 => 8208 [-29 bytes / -0.35%]
other/codesize/test_codesize_cxx_noexcept.jssize: 19928 => 19920 [-8 bytes / -0.04%]
other/codesize/test_codesize_cxx_wasmfs.gzsize: 3413 => 3387 [-26 bytes / -0.76%]
other/codesize/test_codesize_cxx_wasmfs.jssize: 7329 => 7320 [-9 bytes / -0.12%]
other/codesize/test_codesize_files_js_fs.gzsize: 7527 => 7499 [-28 bytes / -0.37%]
other/codesize/test_codesize_files_js_fs.jssize: 18510 => 18501 [-9 bytes / -0.05%]
other/codesize/test_codesize_files_wasmfs.gzsize: 2668 => 2640 [-28 bytes / -1.05%]
other/codesize/test_codesize_files_wasmfs.jssize: 5714 => 5703 [-11 bytes / -0.19%]
other/codesize/test_codesize_hello_O0.gzsize: 7806 => 7785 [-21 bytes / -0.27%]
other/codesize/test_codesize_hello_O0.jssize: 20758 => 20761 [+3 bytes / +0.01%]
other/codesize/test_codesize_hello_O1.gzsize: 2534 => 2510 [-24 bytes / -0.95%]
other/codesize/test_codesize_hello_O1.jssize: 6528 => 6536 [+8 bytes / +0.12%]
other/codesize/test_codesize_hello_O2.gzsize: 2193 => 2170 [-23 bytes / -1.05%]
other/codesize/test_codesize_hello_O2.jssize: 4508 => 4497 [-11 bytes / -0.24%]
other/codesize/test_codesize_hello_O3.gzsize: 2149 => 2128 [-21 bytes / -0.98%]
other/codesize/test_codesize_hello_O3.jssize: 4450 => 4439 [-11 bytes / -0.25%]
other/codesize/test_codesize_hello_Os.gzsize: 2149 => 2128 [-21 bytes / -0.98%]
other/codesize/test_codesize_hello_Os.jssize: 4450 => 4439 [-11 bytes / -0.25%]
other/codesize/test_codesize_hello_Oz.gzsize: 2149 => 2128 [-21 bytes / -0.98%]
other/codesize/test_codesize_hello_Oz.jssize: 4450 => 4439 [-11 bytes / -0.25%]
other/codesize/test_codesize_hello_dylink.gzsize: 11753 => 11735 [-18 bytes / -0.15%]
other/codesize/test_codesize_hello_dylink.jssize: 27782 => 27774 [-8 bytes / -0.03%]
other/codesize/test_codesize_hello_export_nothing.gzsize: 1545 => 1520 [-25 bytes / -1.62%]
other/codesize/test_codesize_hello_export_nothing.jssize: 3338 => 3325 [-13 bytes / -0.39%]
other/codesize/test_codesize_hello_single_file.gzsize: 3673 => 3626 [-47 bytes / -1.28%]
other/codesize/test_codesize_hello_single_file.jssize: 6732 => 6690 [-42 bytes / -0.62%]
other/codesize/test_codesize_hello_wasmfs.gzsize: 2149 => 2128 [-21 bytes / -0.98%]
other/codesize/test_codesize_hello_wasmfs.jssize: 4450 => 4439 [-11 bytes / -0.25%]
other/codesize/test_codesize_libcxxabi_message_O3.gzsize: 1736 => 1713 [-23 bytes / -1.32%]
other/codesize/test_codesize_libcxxabi_message_O3.jssize: 3713 => 3700 [-13 bytes / -0.35%]
other/codesize/test_codesize_libcxxabi_message_O3_standalone.gzsize: 1769 => 1745 [-24 bytes / -1.36%]
other/codesize/test_codesize_libcxxabi_message_O3_standalone.jssize: 3756 => 3743 [-13 bytes / -0.35%]
other/codesize/test_codesize_mem_O3.gzsize: 2190 => 2171 [-19 bytes / -0.87%]
other/codesize/test_codesize_mem_O3.jssize: 4579 => 4569 [-10 bytes / -0.22%]
other/codesize/test_codesize_mem_O3_grow.gzsize: 2342 => 2323 [-19 bytes / -0.81%]
other/codesize/test_codesize_mem_O3_grow.jssize: 4864 => 4854 [-10 bytes / -0.21%]
other/codesize/test_codesize_mem_O3_grow_standalone.gzsize: 2035 => 2013 [-22 bytes / -1.08%]
other/codesize/test_codesize_mem_O3_grow_standalone.jssize: 4277 => 4265 [-12 bytes / -0.28%]
other/codesize/test_codesize_mem_O3_standalone.gzsize: 1999 => 1977 [-22 bytes / -1.10%]
other/codesize/test_codesize_mem_O3_standalone.jssize: 4210 => 4197 [-13 bytes / -0.31%]
other/codesize/test_codesize_mem_O3_standalone_lib.gzsize: 1761 => 1739 [-22 bytes / -1.25%]
other/codesize/test_codesize_mem_O3_standalone_lib.jssize: 3769 => 3756 [-13 bytes / -0.34%]
other/codesize/test_codesize_mem_O3_standalone_narg.gzsize: 1769 => 1745 [-24 bytes / -1.36%]
other/codesize/test_codesize_mem_O3_standalone_narg.jssize: 3756 => 3743 [-13 bytes / -0.35%]
other/codesize/test_codesize_mem_O3_standalone_narg_flto.gzsize: 1769 => 1745 [-24 bytes / -1.36%]
other/codesize/test_codesize_mem_O3_standalone_narg_flto.jssize: 3756 => 3743 [-13 bytes / -0.35%]
other/codesize/test_codesize_minimal_64.gzsize: 1317 => 1290 [-27 bytes / -2.05%]
other/codesize/test_codesize_minimal_64.jssize: 2775 => 2763 [-12 bytes / -0.43%]
other/codesize/test_codesize_minimal_O0.gzsize: 6119 => 6099 [-20 bytes / -0.33%]
other/codesize/test_codesize_minimal_O0.jssize: 16141 => 16143 [+2 bytes / +0.01%]
other/codesize/test_codesize_minimal_O1.gzsize: 1381 => 1352 [-29 bytes / -2.10%]
other/codesize/test_codesize_minimal_O1.jssize: 3240 => 3246 [+6 bytes / +0.19%]
other/codesize/test_codesize_minimal_O2.gzsize: 1247 => 1221 [-26 bytes / -2.09%]
other/codesize/test_codesize_minimal_O2.jssize: 2522 => 2510 [-12 bytes / -0.48%]
other/codesize/test_codesize_minimal_O3.gzsize: 1212 => 1187 [-25 bytes / -2.06%]
other/codesize/test_codesize_minimal_O3.jssize: 2472 => 2460 [-12 bytes / -0.49%]
other/codesize/test_codesize_minimal_Os.gzsize: 1212 => 1187 [-25 bytes / -2.06%]
other/codesize/test_codesize_minimal_Os.jssize: 2472 => 2460 [-12 bytes / -0.49%]
other/codesize/test_codesize_minimal_Oz-ctors.gzsize: 1197 => 1171 [-26 bytes / -2.17%]
other/codesize/test_codesize_minimal_Oz-ctors.jssize: 2451 => 2439 [-12 bytes / -0.49%]
other/codesize/test_codesize_minimal_Oz.gzsize: 1212 => 1187 [-25 bytes / -2.06%]
other/codesize/test_codesize_minimal_Oz.jssize: 2472 => 2460 [-12 bytes / -0.49%]
other/codesize/test_codesize_minimal_esm.gzsize: 1404 => 1298 [-106 bytes / -7.55%]
other/codesize/test_codesize_minimal_esm.jssize: 2928 => 2697 [-231 bytes / -7.89%]
other/codesize/test_codesize_minimal_pthreads.gzsize: 4041 => 3983 [-58 bytes / -1.44%]
other/codesize/test_codesize_minimal_pthreads.jssize: 8380 => 8257 [-123 bytes / -1.47%]
other/codesize/test_codesize_minimal_wasmfs.gzsize: 1212 => 1187 [-25 bytes / -2.06%]
other/codesize/test_codesize_minimal_wasmfs.jssize: 2472 => 2460 [-12 bytes / -0.49%]
other/test_unoptimized_code_size.js.size: 51838 => 51593 [-245 bytes / -0.47%]
other/test_unoptimized_code_size_no_asserts.js.size: 27206 => 26961 [-245 bytes / -0.90%]
other/test_unoptimized_code_size_strict.js.size: 49906 => 49661 [-245 bytes / -0.49%]

Average change: -0.84% (-7.89% - +0.19%)
```
@kleisauke kleisauke force-pushed the simplify-script-dir branch from c457e42 to 65b2a41 Compare April 17, 2025 08:27
@kleisauke kleisauke changed the title Simplify _scriptName / scriptDirectory handling Simplify _scriptName / scriptDirectory handling. NFC Apr 17, 2025
@kleisauke
Copy link
Collaborator Author

BTW, do you know if -sMINIMAL_RUNTIME and -sWASM_WORKERS support the -sEXPORT_ES6 setting? It seems to always use __filename, which won't work in ESM modules.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 17, 2025

BTW, do you know if -sMINIMAL_RUNTIME and -sWASM_WORKERS support the -sEXPORT_ES6 setting? It seems to always use __filename, which won't work in ESM modules.

It seems unlikely that was ever tested. I think its unlike that MINIMAL_RUNTIME and EXPORT_ES6 are used together.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 17, 2025

Out of interest, do any/all of the new tests fail without this change?

The new tests also seem to pass on the main branch. IIRC, the other.test_locate_file_abspath_pthread test used to fail, but it was likely fixed when the pthread *.worker.js file was inlined into the main output file.

Given this, this PR is (likely) a non-functional change now. I'll add the NFC keyword to the title.

Can you add this info the to the PR? i.e. explain why the new tests are useful, even though they pass before and after this PR?

@sbc100
Copy link
Collaborator

sbc100 commented Apr 17, 2025

I love this change BTW. I've tried to do something like this myself several times in the past but always given up. Thanks!

@kleisauke
Copy link
Collaborator Author

Can you add this info the to the PR?

Sure, I've just added another bullet point with this info to the PR description.

I love this change BTW. I've tried to do something like this myself several times in the past but always given up. Thanks!

No problem! :)

@sbc100 sbc100 enabled auto-merge (squash) April 17, 2025 16:41
@sbc100 sbc100 merged commit 4647567 into emscripten-core:main Apr 17, 2025
28 checks passed
@kleisauke kleisauke deleted the simplify-script-dir branch April 17, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbc100 sbc100 sbc100 approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@kleisauke @sbc100