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

perf(node/fs): faster existsSync when not exists #21458

Merged
merged 7 commits into from
Dec 4, 2023
Merged

perf(node/fs): faster existsSync when not exists #21458

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a515f07
Start
dsherret Dec 2, 2023
fcd9888
Merge branch 'main' into deno_exists_sync
dsherret Dec 4, 2023
ac0cdf3
Add tests.
dsherret Dec 4, 2023
7d9d667
Revert I think.
dsherret Dec 4, 2023
537e2ca
Move to ext/node
dsherret Dec 4, 2023
0d38cb8
Self review
dsherret Dec 4, 2023
315b84a
Add inline(always) to be like the other ones
dsherret Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion cli/tests/unit_node/fs_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import { assert, assertThrows } from "../../../test_util/std/assert/mod.ts";
import { join } from "node:path";
import { tmpdir } from "node:os";
import { mkdtempSync, readFileSync, writeFileSync } from "node:fs";
import { existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs";
import { pathToAbsoluteFileUrl } from "../unit/test_util.ts";

Deno.test(
"[node/fs writeFileSync] write file without option",
Expand Down Expand Up @@ -43,3 +44,41 @@ Deno.test(
);
},
);

Deno.test(
"[node/fs existsSync] path",
{ permissions: { read: true } },
function () {
assert(existsSync("cli/tests/testdata/assets/fixture.json"));
},
);

Deno.test(
"[node/fs existsSync] url",
{ permissions: { read: true } },
function () {
assert(existsSync(
pathToAbsoluteFileUrl("cli/tests/testdata/assets/fixture.json"),
));
},
);

Deno.test(
"[node/fs existsSync] no permission",
{ permissions: { read: false } },
function () {
assertThrows(() => {
existsSync("cli/tests/testdata/assets/fixture.json");
}, Deno.errors.PermissionDenied);
},
);

Deno.test(
"[node/fs existsSync] not exists",
{ permissions: { read: true } },
function () {
assertThrows(() => {
Deno.readTextFileSync("bad_filename");
dsherret marked this conversation as resolved.
Show resolved Hide resolved
}, Deno.errors.NotFound);
},
);
1 change: 1 addition & 0 deletions ext/fs/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ deno_core::extension!(deno_fs,
op_fs_remove_async<P>,
op_fs_copy_file_sync<P>,
op_fs_copy_file_async<P>,
op_fs_exists_sync<P>,
op_fs_stat_sync<P>,
op_fs_stat_async<P>,
op_fs_lstat_sync<P>,
Expand Down
17 changes: 17 additions & 0 deletions ext/fs/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,23 @@ where
Ok(SerializableStat::from(stat))
}

#[op2(fast)]
pub fn op_fs_exists_sync<P>(
state: &mut OpState,
#[string] path: String,
) -> Result<bool, AnyError>
where
P: FsPermissions + 'static,
{
let path = PathBuf::from(path);
state
.borrow_mut::<P>()
// currently only used by node:fs
.check_read(&path, "node:fs.existsSync()")?;
dsherret marked this conversation as resolved.
Show resolved Hide resolved
let fs = state.borrow::<FileSystemRc>();
Ok(fs.lstat_sync(&path).is_ok())
}

#[op2]
#[string]
pub fn op_fs_realpath_sync<P>(
Expand Down
9 changes: 2 additions & 7 deletions ext/node/polyfills/_fs/_fs_exists.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

// TODO(petamoriken): enable prefer-primordials for node polyfills
// deno-lint-ignore-file prefer-primordials

const core = globalThis.__bootstrap.core;
import { pathFromURL } from "ext:deno_web/00_infra.js";

type ExistsCallback = (exists: boolean) => void;
Expand Down Expand Up @@ -35,10 +35,5 @@ Object.defineProperty(exists, kCustomPromisifiedSymbol, {
*/
export function existsSync(path: string | URL): boolean {
path = path instanceof URL ? pathFromURL(path) : path;
try {
Deno.lstatSync(path);
return true;
} catch (_err) {
return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

One other thing this fixes is previously this would supress permission errors.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC this was done on purpose, but seems fishy. @kt3k any idea here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If intentional, my guess would be to reduce permission prompts, but I don't think we should do that because it causes subtle bugs.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it was for avoiding to throw 'Not a directory' error when called with non-existing dir denoland/std#2494

}
return core.ops.op_fs_exists_sync(path);
}