fs: add isFile and isDirectory methods #46345

debadree25 · 2023-01-25T07:36:19Z

Attempted to add a isFile, isDirectory apis as per the referenced issue, basically this would avoid the creation of the stats object so I think this should provide somewhat of a speed up, I believe we could speedup from the cpp side too but not much luck in finding what to improve there, any help would be appreciated!

Refs: nodejs/performance#46

debadree25 · 2023-01-25T07:38:07Z

Attempting to add a benchmark, any preliminary reviews would be appreciated

cc @anonrig

tniessen · 2023-01-25T10:30:05Z

lib/fs.js

+  const stats = binding.stat(pathModule.toNamespacedPath(path), true, undefined, ctx);
+  if (hasNoEntryError(ctx)) {
+    return false;
+  }
+  return validateIfFileFromStats(stats);


What if a different error occurs?

Have updated the error handling

Same problem in isDirectory.

tniessen · 2023-01-25T10:34:53Z

basically this would avoid the creation of the stats object so I think this should provide somewhat of a speed up, I believe we could speedup from the cpp side too but not much luck in finding what to improve there

Such claims really need realistic benchmarks. I would expect sync disk I/O to far outweigh the cost of creating the JS object, except perhaps when repeatedly accessing the same disk block on an SSD in a loop.

This also promotes the anti-pattern of fs operations that result in race conditions.

debadree25 · 2023-01-25T10:52:09Z

Such claims really need realistic benchmarks. I would expect sync disk I/O to far outweigh the cost of creating the JS object, except perhaps when repeatedly accessing the same disk block on an SSD in a loop.

Have added a benchmark for this, should also commit the comparison benchmark file?
also how could we make it realistic if you could say? maybe randomly open multiple files?

debadree25 · 2023-01-25T10:55:05Z

have run the benchmarks on my local machine the results look something like this:

fs/bench-isFile-stat.js
fs/bench-isFile-stat.js fileName="/Users/debadreechatterjee/Documents/Personal/node/benchmark/fs/bench-isFile-stat.js" n=1000000: 192,685.4785343927
fs/bench-isFile-stat.js fileName="./not-exist.txt" n=1000000: 444,116.0411600369

fs/bench-isFile.js
fs/bench-isFile.js fileName="/Users/debadreechatterjee/Documents/Personal/node/benchmark/fs/bench-isFile.js" n=1000000: 231,165.77255325063
fs/bench-isFile.js fileName="./not-exist.txt" n=1000000: 484,097.7516883041

So looks somewhat faster?

also i am unable to understand how to run the benchmark with statistics for proper comparison so any help for that would be appreciated

debadree25 · 2023-01-25T10:56:30Z

This also promotes the anti-pattern of fs operations that result in race conditions.

couldnt get you here 😅😅 could you elaborate?

tniessen · 2023-01-25T12:05:41Z

also how could we make it realistic if you could say? maybe randomly open multiple files?

That is going to be tricky. OS kernels are extremely effective at caching inodes and since this feature does not depend on actual file contents, any I/O during a benchmark will likely hit the inode cache. And even if it doesn't, it'll probably hit the kernel's disk cache. And if it doesn't, most SSDs maintain their own internal cache. In other words, as long as the benchmark accesses a predictable and/or smallish part of the disk only (and assuming the disk is not a shared medium, network-attached storage, etc.), it won't reflect actual I/O times.

This also promotes the anti-pattern of fs operations that result in race conditions.

couldnt get you here 😅😅 could you elaborate?

APIs such as fs.exists are problematic because they often introduce race conditions in programs. The result is often useless because the file might be deleted (or created) before the application is even able to observe the result. There are some justified use cases, of course, but it is an anti-pattern in general. See #39968 for a related discussion.

debadree25 · 2023-01-25T12:12:11Z

That is going to be tricky. OS kernels are extremely effective at caching inodes and since this feature does not depend on actual file contents, any I/O during a benchmark will likely hit the inode cache. And even if it doesn't, it'll probably hit the kernel's disk cache. And if it doesn't, most SSDs maintain their own internal cache. In other words, as long as the benchmark accesses a predictable and/or smallish part of the disk only (and assuming the disk is not a shared medium, network-attached storage, etc.), it won't reflect actual I/O times.

Thank you so much for explaining much appreciated

debadree25 · 2023-01-25T12:14:13Z

APIs such as fs.exists are problematic because they often introduce race conditions in programs. The result is often useless because the file might be deleted (or created) before the application is even able to observe the result. There are some justified use cases, of course, but it is an anti-pattern in general. See #39968 for a related discussion.

Went through the discussion it seems like its DX vs Discouraging anti-patterns issue and I don't think there is any consensus on that yet?
so should I close this PR or leave it open for any discussions for now?

debadree25 · 2023-01-25T12:26:22Z

Also for the original intent of the issue nodejs/performance#46 (comment) of speeding up checking during module resolution do you think this could speed it up? since (i think please correct me if am wrong 😅) during module resolution, we would be accessing predictable parts of the disk multiple times?

cjihrig

I'll wait for benchmark numbers, but I really don't think we should add these as public API.

cjihrig · 2023-01-25T14:52:32Z

lib/fs.js

+  isFile,
+  isDirectory,


It seems like these should have "sync" as part of their name.

Refactoring

Also, could you say how I could post the comparison benchmarks?

RaisinTen · 2023-01-25T15:53:43Z

during module resolution

If you're referring to the ESM loader alone, I wonder why we aren't using the InternalModuleStat function there like we are using it in the CJS loader. Maybe that would speed things up without exposing more public APIs 🤔

debadree25 · 2023-01-25T16:03:53Z

during module resolution

If you're referring to the ESM loader alone, I wonder why we aren't using the InternalModuleStat function there like we are using it in the CJS loader. Maybe that would speed things up without exposing more public APIs 🤔

Oh ok didn't know this could explore in a separate PR I guess, I created this one for the general comment in nodejs/performance#46 (comment)

debadree25 · 2023-01-25T16:38:57Z

Ok I tried replacing the implementation of isFileSync with InternalModuleStat the results seem to be significantly faster now especially, if a file doesn't exist

Running on my local machine:
using isFileSync:

fs/bench-isFile.js fileName="/Users/debadreechatterjee/Documents/Personal/node/benchmark/fs/bench-isFile.js" n=1000000: 261,749.895896177
fs/bench-isFile.js fileName="./not-exist.txt" n=1000000: 729,323.8410444844

and using statSync().isFile()

fs/bench-isFile-stat.js fileName="/Users/debadreechatterjee/Documents/Personal/node/benchmark/fs/bench-isFile-stat.js" n=1000000: 198,631.47703315382
fs/bench-isFile-stat.js fileName="./not-exist.txt" n=1000000: 467,543.81614458084

@RaisinTen

Refs: nodejs/performance#46

debadree25 · 2023-02-15T12:03:06Z

since there isn't any consensus on making this change yet, and also the benchmark perf isn't hugely better closing this for now

I think it may be possible to improve the stats object itself it was mentioned in nodejs/performance#39 (comment) but not sure how to do that 😅 but it seems that way can really yield perf gains ex of bun https://twitter.com/jarredsumner/status/1622839218094641152

tniessen reviewed Jan 25, 2023

View reviewed changes

cjihrig reviewed Jan 25, 2023

View reviewed changes

climba03003 mentioned this pull request Jan 31, 2023

Improve Etag performance by caching fs.stats fastify/send#36

Open

2 tasks

debadree25 mentioned this pull request Feb 9, 2023

Module resolution nodejs/performance#39

Open

debadree25 added 6 commits February 15, 2023 17:12

fs: add isFile and isDirectory methods

4e76eab

Refs: nodejs/performance#46

fixup! change error handling

c4ac04a

fixup! isFile benchmark

0cdb468

fixup! error handling

45d6c69

fixup! error handling again

46847ea

fixup! refactored to include sync

f20092b

debadree25 force-pushed the ft/add-isFile-isDirectory branch from 2d0f314 to f20092b Compare February 15, 2023 11:44

debadree25 closed this Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: add isFile and isDirectory methods #46345

fs: add isFile and isDirectory methods #46345

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023

tniessen Jan 25, 2023

debadree25 Jan 25, 2023

tniessen Jan 25, 2023

debadree25 Jan 25, 2023

tniessen commented Jan 25, 2023

debadree25 commented Jan 25, 2023 •

edited

Loading

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023 •

edited

Loading

tniessen commented Jan 25, 2023

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023 •

edited

Loading

debadree25 commented Jan 25, 2023

cjihrig left a comment •

edited

Loading

cjihrig Jan 25, 2023

debadree25 Jan 25, 2023

debadree25 Jan 25, 2023

debadree25 Jan 25, 2023

RaisinTen commented Jan 25, 2023

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023

debadree25 commented Feb 15, 2023

fs: add isFile and isDirectory methods #46345

fs: add isFile and isDirectory methods #46345

Conversation

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Jan 25, 2023

debadree25 commented Jan 25, 2023 • edited Loading

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023 • edited Loading

tniessen commented Jan 25, 2023

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023 • edited Loading

debadree25 commented Jan 25, 2023

cjihrig left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaisinTen commented Jan 25, 2023

debadree25 commented Jan 25, 2023

debadree25 commented Jan 25, 2023

debadree25 commented Feb 15, 2023

debadree25 commented Jan 25, 2023 •

edited

Loading

debadree25 commented Jan 25, 2023 •

edited

Loading

debadree25 commented Jan 25, 2023 •

edited

Loading

cjihrig left a comment •

edited

Loading