node
node copied to clipboard
nodejs
lib: merge cjs and esm package json reader caches
This PR contains a refactor to merge package.json reader caches in both CJS and ESM.
I extracted the relevant parts from https://github.com/nodejs/node/pull/47991.
cc @nodejs/loaders @nodejs/modules
PS: This PR has no effect on execution speed.
hyperfine './out/Release/node ../fastify/fastify.js' 'node ../fastify/fastify.js' 'bun ../fastify/fastify.js' -i --warmup 10
Benchmark 1: ./out/Release/node ../fastify/fastify.js
Time (mean ± σ): 87.5 ms ± 1.3 ms [User: 85.0 ms, System: 19.7 ms]
Range (min … max): 84.3 ms … 89.5 ms 32 runs
Benchmark 2: node ../fastify/fastify.js
Time (mean ± σ): 87.8 ms ± 1.3 ms [User: 82.2 ms, System: 21.3 ms]
Range (min … max): 84.5 ms … 91.5 ms 33 runs
Benchmark 3: bun ../fastify/fastify.js
Time (mean ± σ): 98.1 ms ± 0.9 ms [User: 83.9 ms, System: 18.9 ms]
Range (min … max): 96.9 ms … 101.4 ms 29 runs
Summary
./out/Release/node ../fastify/fastify.js ran
1.00 ± 0.02 times faster than node ../fastify/fastify.js
1.12 ± 0.02 times faster than bun ../fastify/fastify.js
@nodejs/build @nodejs/platform-aix I don't have access to aix platform, and can't figure out why the test fails on AIX. Any suggestions?
@nodejs/build @nodejs/platform-aix I don't have access to
aixplatform, and can't figure out why the test fails on AIX. Any suggestions?
FWIW the error is SyntaxError: Error parsing /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/fixtures/packages/is-dir/package.json: Unexpected token 'U', "U�."... is not valid JSON
15:23:18 not ok 2008 parallel/test-module-loading-error
15:23:18 ---
15:23:18 duration_ms: 230.33100
15:23:18 severity: fail
15:23:18 exitcode: 1
15:23:18 stack: |-
15:23:18 node:assert:635
15:23:18 throw err;
15:23:18 ^
15:23:18
15:23:18 AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
15:23:18 + actual - expected
15:23:18
15:23:18 Comparison {
15:23:18 + message: `Error parsing /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/fixtures/packages/is-dir/package.json: Unexpected token 'U', "U�.\x00\x00\x00\x00\x00\x00\x00"... is not valid JSON`
15:23:18 - code: 'MODULE_NOT_FOUND',
15:23:18 - message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/
15:23:18 }
15:23:18 at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/parallel/test-module-loading-error.js:87:8)
15:23:18 at Module._compile (node:internal/modules/cjs/loader:1233:14)
15:23:18 at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
15:23:18 at Module.load (node:internal/modules/cjs/loader:1091:32)
15:23:18 at Module._load (node:internal/modules/cjs/loader:938:12)
15:23:18 at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
15:23:18 at node:internal/main/run_main_module:23:47 {
15:23:18 generatedMessage: true,
15:23:18 code: 'ERR_ASSERTION',
15:23:18 actual: SyntaxError: Error parsing /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/fixtures/packages/is-dir/package.json: Unexpected token 'U', "U�."... is not valid JSON
15:23:18 at parse (<anonymous>)
15:23:18 at Object.read (node:internal/modules/package_json_reader:63:16)
15:23:18 at readPackage (node:internal/modules/cjs/loader:362:28)
15:23:18 at tryPackage (node:internal/modules/cjs/loader:401:15)
15:23:18 at Module._findPath (node:internal/modules/cjs/loader:665:18)
15:23:18 at Module._resolveFilename (node:internal/modules/cjs/loader:1034:27)
15:23:18 at Module._load (node:internal/modules/cjs/loader:901:27)
15:23:18 at Module.require (node:internal/modules/cjs/loader:1115:19)
15:23:18 at require (node:internal/modules/helpers:121:18)
15:23:18 at assert.throws.code (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/parallel/test-module-loading-error.js:88:11) {
15:23:18 path: '/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/fixtures/packages/is-dir/package.json'
15:23:18 },
15:23:18 expected: {
15:23:18 code: 'MODULE_NOT_FOUND',
15:23:18 message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/
15:23:18 },
15:23:18 operator: 'throws'
15:23:18 }
15:23:18
15:23:18 Node.js v21.0.0-pre
15:23:18 ...
The message is truncated -- what is happening is that reading test/fixtures/packages/is-dir/package.json (which is a directory) is succeeding on AIX:
> fs.readFileSync('/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/fixtures/packages/is-dir/package.json', { encoding: 'utf8'})
'U�.\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00U�..\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00U�.placeholder\x00\x00'
This is a difference between AIX and Linux (which throws an EISDIR error), see https://github.com/libuv/libuv/pull/2025 where libuv removed the special handling. That PR also mentions BSD although it doesn't look like the same CI failed for FreeBSD 🤷.
@richardlau Thanks for the help. I'll disable the last test case for folder on AIX.
@richardlau Thanks for the help. I'll disable the last test case for folder on AIX.
Isn't this a regression? The test worked before this PR.
Isn't this a regression? The test worked before this PR.
Yes it is a regression. I just want to make sure this is the only issue. I’ll take a look at AIX.
So, let me summarize the issue here:
Previously, CJS loader was checking if any of the keys existed in a package.json, and if not tried to JSONParse a {} value (Ref: https://github.com/nodejs/node/pull/48477/files#diff-5b5902273122e094ff474fda358605ffa45a4a58b51cd0bf4c1acb93779df142L369), causing a ERR_MODULE_NOT_FOUND error on a directory file (which only succeeds for AIX)
With the current changes, I removed the need to check if any of the keys existed (such as name, type etc). This worked for all platforms except AIX, because AIX is the only platform that resolves a folder as a file (Ref: https://github.com/nodejs/node/pull/48477#issuecomment-1604586650)
This particular issue on AIX requires us to traverse the package.json file in the C++ while causing a little bit of performance impact, but most importantly forces us to implement this particular code block: https://github.com/nodejs/node/pull/48477/files#diff-70e3325bd2115867617ae2a16321c5de53c070ff2841976fe5b849675a5111e6L1087
As a summary, on AIX instead of throwing ERR_MODULE_NOT_FOUND on a directory, it now throws a Error parsing ... error (due to the fact that the file read operation succeeds on a directory entry).
Should we add a semver-major label to this pull request, or is it ok to omit it?
cc @nodejs/tsc @nodejs/platform-aix @nodejs/loaders
As a summary, on AIX instead of throwing
ERR_MODULE_NOT_FOUNDon a directory, it now throws aError parsing ...error (due to the fact that the file read operation succeeds on a directory entry).Should we add a
semver-majorlabel to this pull request, or is it ok to omit it?
I would recommend making the necessary checks so this PR behaves as previously for each platform (and too bad for performance), and then land a semver-major PR that optimize and simplify things further. I think the consensus is that changing a error code is deemed as semver-major.
I'll revert some changes regarding AIX to not make this pull request a semver-major change, and after this has been merged, I'll follow-up with removing the change specific for AIX.
The failure on the worker seems to be related. For the Windows failure, maybe @nodejs/platform-windows can help?