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

Add a checkOpExists helper for common code pattern. NFC #23305

Merged
merged 10 commits into from
Jan 10, 2025
Merged

Add a checkOpExists helper for common code pattern. NFC #23305

merged 10 commits into from
Jan 10, 2025

Conversation

hoodmane
Copy link
Collaborator

@hoodmane hoodmane commented Jan 6, 2025

We frequently check if a node op is present, if not raise a specific error, if it is present call it. This factors out this common pattern.

@hoodmane
Add a doNodeOp helper for common code pattern. NFC
8c27f3e 
We frequently check if a node op is present, if not raise a specific error,
if it is present call it. This factors out this common pattern.
hoodmane
hoodmane commented Jan 6, 2025
View reviewed changes
src/library_fs.js Outdated Show resolved Hide resolved
hoodmane
hoodmane commented Jan 6, 2025
View reviewed changes
src/library_fs.js
Comment on lines -1019 to -1023
if (!node.node_ops.setattr) {
throw new FS.ErrnoError({{{ cDefs.EPERM }}});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This changes which error we throw if there are multiple problems.

hoodmane
hoodmane commented Jan 6, 2025
View reviewed changes
src/library_fs.js
@@ -1044,7 +1033,7 @@ FS.staticInit();
utime(path, atime, mtime) {
var lookup = FS.lookupPath(path, { follow: true });
var node = lookup.node;
node.node_ops.setattr(node, {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously there was no error handling for setattr being missing, so if we called utime on a read only node it would crash.

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

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

```
other/codesize/test_codesize_cxx_ctors1.gzsize: 8365 => 8379 [+14 bytes / +0.17%]
other/codesize/test_codesize_cxx_ctors1.jssize: 20373 => 20384 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8347 => 8365 [+18 bytes / +0.22%]
other/codesize/test_codesize_cxx_ctors2.jssize: 20341 => 20352 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except.gzsize: 9369 => 9376 [+7 bytes / +0.07%]
other/codesize/test_codesize_cxx_except.jssize: 24141 => 24152 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8328 => 8340 [+12 bytes / +0.14%]
other/codesize/test_codesize_cxx_except_wasm.jssize: 20267 => 20278 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_except_wasm_exnref.gzsize: 8328 => 8340 [+12 bytes / +0.14%]
other/codesize/test_codesize_cxx_except_wasm_exnref.jssize: 20267 => 20278 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_lto.gzsize: 8360 => 8372 [+12 bytes / +0.14%]
other/codesize/test_codesize_cxx_lto.jssize: 20397 => 20408 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_mangle.gzsize: 9374 => 9381 [+7 bytes / +0.07%]
other/codesize/test_codesize_cxx_mangle.jssize: 24142 => 24153 [+11 bytes / +0.05%]
other/codesize/test_codesize_cxx_noexcept.gzsize: 8365 => 8379 [+14 bytes / +0.17%]
other/codesize/test_codesize_cxx_noexcept.jssize: 20373 => 20384 [+11 bytes / +0.05%]
other/codesize/test_codesize_files_js_fs.gzsize: 7655 => 7672 [+17 bytes / +0.22%]
other/codesize/test_codesize_files_js_fs.jssize: 18867 => 18879 [+12 bytes / +0.06%]

Average change: +0.10% (+0.05% - +0.22%)
```
@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 6, 2025

I'm surprised that this increased code size by a few bytes...

sbc100
sbc100 reviewed Jan 6, 2025
View reviewed changes
src/library_fs.js Outdated Show resolved Hide resolved
sbc100
sbc100 reviewed Jan 9, 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.

Nice! That looks much better. Can you rename to to something less overloaded though? checkExists sounds like it will check for file existence, no?

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 9, 2025

checkOpExists?

sbc100
sbc100 reviewed Jan 9, 2025
View reviewed changes
src/library_fs.js Outdated
throw new FS.ErrnoError({{{ cDefs.ENOTDIR }}});
}
return node.node_ops.readdir(node);
var readdir = FS.checkExists(node.node_ops.readdir, {{{ cDefs.ENOTDIR }}});
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about checkAttribute? Or getAttribute? Or maybe checkFSOp? Or checkOpExists?

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 like checkOpExists the best.

sbc100
sbc100 reviewed Jan 9, 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.

lgtm % name bikeshedding. Its kind of shame that FS is a public API.. there is no easy way for us to stop external folks calling this function.

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 9, 2025

Indeed.

sbc100
sbc100 reviewed Jan 9, 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.

An alternative to this would be to force all FS impls to implement all operations, and have them report correct errors when they don't, but maybe thats a bigger change..

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 9, 2025

I find it quite convenient that they default to ENOSYS personally.

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 9, 2025

Though maybe getattr could be required, and read only file systems can be allowed to drop setattr?

@hoodmane hoodmane changed the title Add a doNodeOp helper for common code pattern. NFC Add a checkOpExists helper for common code pattern. NFC Jan 9, 2025
@hoodmane
Add back enoent check
f126175
@hoodmane
Merge branch 'main' into do-node-op
57198a4
@hoodmane
Automatic rebaseline of codesize expectations. NFC
15ae885 
This is an automatic change generated by tools/maint/rebaseline_tests.py.

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

```
other/codesize/test_codesize_cxx_ctors1.gzsize: 8343 => 8350 [+7 bytes / +0.08%]
other/codesize/test_codesize_cxx_ctors1.jssize: 20271 => 20272 [+1 bytes / +0.00%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8325 => 8334 [+9 bytes / +0.11%]
other/codesize/test_codesize_cxx_ctors2.jssize: 20239 => 20240 [+1 bytes / +0.00%]
other/codesize/test_codesize_cxx_except.gzsize: 9346 => 9349 [+3 bytes / +0.03%]
other/codesize/test_codesize_cxx_except.jssize: 24039 => 24040 [+1 bytes / +0.00%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8288 => 8296 [+8 bytes / +0.10%]
other/codesize/test_codesize_cxx_except_wasm.jssize: 20164 => 20165 [+1 bytes / +0.00%]
other/codesize/test_codesize_cxx_except_wasm_exnref.gzsize: 8288 => 8296 [+8 bytes / +0.10%]
other/codesize/test_codesize_cxx_except_wasm_exnref.jssize: 20164 => 20165 [+1 bytes / +0.00%]
other/codesize/test_codesize_cxx_lto.gzsize: 8357 => 8364 [+7 bytes / +0.08%]
other/codesize/test_codesize_cxx_lto.jssize: 20347 => 20348 [+1 bytes / +0.00%]
other/codesize/test_codesize_cxx_mangle.gzsize: 9351 => 9355 [+4 bytes / +0.04%]
other/codesize/test_codesize_cxx_mangle.jssize: 24039 => 24040 [+1 bytes / +0.00%]
other/codesize/test_codesize_cxx_noexcept.gzsize: 8343 => 8350 [+7 bytes / +0.08%]
other/codesize/test_codesize_cxx_noexcept.jssize: 20271 => 20272 [+1 bytes / +0.00%]
other/codesize/test_codesize_files_js_fs.gzsize: 7650 => 7652 [+2 bytes / +0.03%]
other/codesize/test_codesize_files_js_fs.jssize: 18817 => 18819 [+2 bytes / +0.01%]

Average change: +0.04% (+0.00% - +0.11%)
```
@sbc100 sbc100 merged commit 723eeaf into emscripten-core:main Jan 10, 2025
29 checks passed
@sbc100
Copy link
Collaborator

sbc100 commented Jan 10, 2025

Oh, lets try to remember to prefix the PR titles for these changes with [FS]?

@hoodmane hoodmane deleted the do-node-op branch January 12, 2025 11:00
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
@hoodmane @sbc100