statSync without throwIfNoEntry: false causes great performance impact when processing typescript imports

[chenxinyanc]

As part of the effort to reduce our project's lint time, I've profiled eslint process to find out the hot path is on the following line of the isFile function

eslint-import-resolver-typescript/src/index.ts

Lines 244 to 250 in d8dd3ca

	const isFile = (path?: string | undefined): path is string => {
	try {
	return !!path && fs.statSync(path).isFile()
	} catch {
	return false
	}
	}

Please note that allowing statSync to throw error for non-existent files will take a lot of precious CPU time generating a user readable stack trace (captureLargerStackTrace) only to be discarded by the try...catch block in this isFile function. Is it possible to use throwIfNoEntry in this case?

 const isFile = (path?: string | undefined): path is string => { 
     return !!path && fs.statSync(path, { throwIfNoEntry: false })?.isFile();
 } 

I've tried to monkey-patch the package with the following diff (generated by yarn v2):

diff --git a/lib/index.cjs b/lib/index.cjs
index c0e3126fdbb2bb1e3602e0aea635032d521a2b30..7c719c7c661e9b6fbea3f1f4cd1fb1b03e28a720 100644
--- a/lib/index.cjs
+++ b/lib/index.cjs
@@ -221,7 +221,7 @@ function removeQuerystring(id) {
 }
 const isFile = (path2) => {
   try {
-    return !!path2 && fs__default["default"].statSync(path2).isFile();
+    return !!path2 && fs__default["default"].statSync(path2, { throwIfNoEntry: false })?.isFile();
   } catch (e) {
     return false;
   }
diff --git a/lib/index.js b/lib/index.js
index b4e8fb2bb153f5ee53ec4b8041ba8abd64eed126..74e7b09c7d83c3d2263710c7b001aee7003c2ff9 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -140,7 +140,7 @@ function removeQuerystring(id) {
 }
 const isFile = (path) => {
     try {
-        return !!path && fs.statSync(path).isFile();
+        return !!path && fs.statSync(path, { throwIfNoEntry: false })?.isFile();
     }
     catch {
         return false;

Here is the time consumption difference in a project with 478 files linted.

BEFORE PATCH

Rule                                             |  Time (ms) | Relative
:------------------------------------------------|-----------:|--------:
import/namespace                                 | 181791.730 |    95.1%
indent                                           |   1612.314 |     0.8%
import/order                                     |   1407.100 |     0.7%
import/no-named-as-default                       |   1147.400 |     0.6%
@typescript-eslint/restrict-template-expressions |    631.019 |     0.3%
@typescript-eslint/naming-convention             |    596.411 |     0.3%
react/boolean-prop-naming                        |    341.655 |     0.2%
import/no-self-import                            |    329.158 |     0.2%
react/no-direct-mutation-state                   |    226.822 |     0.1%
import/no-unresolved                             |    209.255 |     0.1%

AFTER PATCH

Rule                                             | Time (ms) | Relative
:------------------------------------------------|----------:|--------:
import/namespace                                 | 62911.965 |    88.4%
indent                                           |  1658.634 |     2.3%
import/order                                     |  1325.994 |     1.9%
@typescript-eslint/restrict-template-expressions |   679.140 |     1.0%
@typescript-eslint/naming-convention             |   606.596 |     0.9%
react/boolean-prop-naming                        |   329.449 |     0.5%
react/no-direct-mutation-state                   |   240.103 |     0.3%
import/no-named-as-default                       |   237.946 |     0.3%
import/no-self-import                            |   227.392 |     0.3%
@typescript-eslint/no-redeclare                  |   195.496 |     0.3%

Time consumed is reduced to around 1/2.8 by this little patch.

I don't have time for a clean repro for this, but if you have more data points by applying this patch, please post it here. It would be helpful. Thanks!

[JounQin]

@chenxinyanc Thanks a lot for your investigation!

Does this mean the try/catch block is unnecessary if throwIfNoEntry: false is set?

[JounQin]

I just notice webpack/enhanced-resolve#316, so a PR is very welcome!

return !!(path && fs.statSync(path, { throwIfNoEntry: false })?.isFile())

[chenxinyanc]

@chenxinyanc Thanks a lot for your investigation!

Does this mean the try/catch block is unnecessary if throwIfNoEntry: false is set?

@JounQin I suppose so (if we have the lesure to be aggressive). There is no documentation on other Errors statSync may throw, so if there is any other Error, it would be "unexpected".

// ENOENT will cause statSync to return undefined if throwIfNoEntry is false.

Welcome to Node.js v14.17.6.
Type ".help" for more information.
> const fs = require('fs')
undefined
> fs.statSync("???")
Uncaught Error: ENOENT: no such file or directory, stat '???'
    at Object.statSync (fs.js:1127:3) {
  errno: -4058,
  syscall: 'stat',
  code: 'ENOENT',
  path: '???'
}
> fs.statSync("\\\\?\\TEST")
Uncaught Error: ENOENT: no such file or directory, stat '\\?\TEST'
    at Object.statSync (fs.js:1127:3) {
  errno: -4058,
  syscall: 'stat',
  code: 'ENOENT',
  path: '\\\\?\\TEST'
}
> fs.statSync("\\\\.\\TEST")
Uncaught Error: ENOENT: no such file or directory, stat '\\.\TEST'
    at Object.statSync (fs.js:1127:3) {
  errno: -4058,
  syscall: 'stat',
  code: 'ENOENT',
  path: '\\\\.\\TEST'
}

[chenxinyanc]

I think I can work on a PR later today.