From fe6b124dc76f3cf57bf648f5731f2a5f71f768bd Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sat, 20 May 2023 20:32:02 -0400 Subject: [PATCH] src: improve package.json reader performance --- lib/internal/modules/cjs/loader.js | 31 +--- lib/internal/modules/esm/package_config.js | 72 ++------- lib/internal/modules/esm/resolve.js | 5 +- lib/internal/modules/package_json_reader.js | 55 ++++++- src/node_errors.h | 1 + src/node_file.cc | 156 ++++++++++++++++---- test/es-module/test-esm-invalid-pjson.js | 29 ---- test/fixtures/errors/force_colors.snapshot | 8 +- test/parallel/test-module-binding.js | 43 +++--- 9 files changed, 220 insertions(+), 180 deletions(-) delete mode 100644 test/es-module/test-esm-invalid-pjson.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 60fa7e79d19693..6c1ac4b4dd9981 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -82,7 +82,6 @@ const { pendingDeprecate, emitExperimentalWarning, kEmptyObject, - filterOwnProperties, setOwnProperty, getLazy, } = require('internal/util'); @@ -353,36 +352,10 @@ function initializeCJS() { // -> a. // -> a/index. -const packageJsonCache = new SafeMap(); - function readPackage(requestPath) { const jsonPath = path.resolve(requestPath, 'package.json'); - - const existing = packageJsonCache.get(jsonPath); - if (existing !== undefined) return existing; - - const result = packageJsonReader.read(jsonPath); - const json = result.containsKeys === false ? '{}' : result.string; - if (json === undefined) { - packageJsonCache.set(jsonPath, false); - return false; - } - - try { - const filtered = filterOwnProperties(JSONParse(json), [ - 'name', - 'main', - 'exports', - 'imports', - 'type', - ]); - packageJsonCache.set(jsonPath, filtered); - return filtered; - } catch (e) { - e.path = jsonPath; - e.message = 'Error parsing ' + jsonPath + ': ' + e.message; - throw e; - } + // Return undefined or the filtered package.json as a JS object + return packageJsonReader.read(jsonPath); } let _readPackage = readPackage; diff --git a/lib/internal/modules/esm/package_config.js b/lib/internal/modules/esm/package_config.js index dc3c37f6042333..ab3f29a54ec504 100644 --- a/lib/internal/modules/esm/package_config.js +++ b/lib/internal/modules/esm/package_config.js @@ -1,18 +1,10 @@ 'use strict'; const { - JSONParse, - ObjectPrototypeHasOwnProperty, SafeMap, StringPrototypeEndsWith, } = primordials; const { URL, fileURLToPath } = require('internal/url'); -const { - ERR_INVALID_PACKAGE_CONFIG, -} = require('internal/errors').codes; - -const { filterOwnProperties } = require('internal/util'); - /** * @typedef {string | string[] | Record} Exports @@ -42,59 +34,23 @@ function getPackageConfig(path, specifier, base) { return existing; } const packageJsonReader = require('internal/modules/package_json_reader'); - const source = packageJsonReader.read(path).string; - if (source === undefined) { - const packageConfig = { - pjsonPath: path, - exists: false, - main: undefined, - name: undefined, - type: 'none', - exports: undefined, - imports: undefined, - }; - packageJSONCache.set(path, packageConfig); - return packageConfig; - } - - let packageJSON; - try { - packageJSON = JSONParse(source); - } catch (error) { - throw new ERR_INVALID_PACKAGE_CONFIG( - path, - (base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier), - error.message, - ); - } - - let { imports, main, name, type } = filterOwnProperties(packageJSON, ['imports', 'main', 'name', 'type']); - const exports = ObjectPrototypeHasOwnProperty(packageJSON, 'exports') ? packageJSON.exports : undefined; - if (typeof imports !== 'object' || imports === null) { - imports = undefined; - } - if (typeof main !== 'string') { - main = undefined; - } - if (typeof name !== 'string') { - name = undefined; - } - // Ignore unknown types for forwards compatibility - if (type !== 'module' && type !== 'commonjs') { - type = 'none'; - } + const result = packageJsonReader.read(path); + const packageJSON = result ?? { + main: undefined, + name: undefined, + type: 'none', + exports: undefined, + imports: undefined, + }; - const packageConfig = { + const json = { + __proto__: null, pjsonPath: path, - exists: true, - main, - name, - type, - exports, - imports, + exists: result !== undefined, + ...packageJSON, }; - packageJSONCache.set(path, packageConfig); - return packageConfig; + packageJSONCache.set(path, json); + return json; } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 927b118f8ede2b..3b790923c1ad99 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -734,8 +734,7 @@ function packageResolve(specifier, base, conditions) { const packageConfig = getPackageScopeConfig(base); if (packageConfig.exists) { const packageJSONUrl = pathToFileURL(packageConfig.pjsonPath); - if (packageConfig.name === packageName && - packageConfig.exports !== undefined && packageConfig.exports !== null) { + if (packageConfig.name === packageName && packageConfig.exports !== undefined) { return packageExportsResolve( packageJSONUrl, packageSubpath, packageConfig, base, conditions); } @@ -760,7 +759,7 @@ function packageResolve(specifier, base, conditions) { // Package match. const packageConfig = getPackageConfig(packageJSONPath, specifier, base); - if (packageConfig.exports !== undefined && packageConfig.exports !== null) { + if (packageConfig.exports !== undefined) { return packageExportsResolve( packageJSONUrl, packageSubpath, packageConfig, base, conditions); } diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js index bb175d0df54c04..867ee58f44adbd 100644 --- a/lib/internal/modules/package_json_reader.js +++ b/lib/internal/modules/package_json_reader.js @@ -1,6 +1,10 @@ 'use strict'; -const { SafeMap } = primordials; +const { + JSONParse, + JSONStringify, + SafeMap, +} = primordials; const { internalModuleReadJSON } = internalBinding('fs'); const { pathToFileURL } = require('url'); const { toNamespacedPath } = require('path'); @@ -10,30 +14,67 @@ const cache = new SafeMap(); let manifest; /** - * + * Returns undefined for all failure cases. * @param {string} jsonPath + * @returns {{ + * name?: string, + * main?: string, + * exports?: string | Record, + * imports?: string | Record, + * type: 'commonjs' | 'module' | 'none' | unknown, + * } | undefined} */ function read(jsonPath) { if (cache.has(jsonPath)) { return cache.get(jsonPath); } - const { 0: string, 1: containsKeys } = internalModuleReadJSON( + const { + 0: includesKeys, + 1: name, + 2: main, + 3: exports, + 4: imports, + 5: type, + 6: parseExports, + 7: parseImports, + } = internalModuleReadJSON( toNamespacedPath(jsonPath), ); - const result = { string, containsKeys }; - const { getOptionValue } = require('internal/options'); - if (string !== undefined) { + + let result; + + if (includesKeys !== undefined) { + result = { + __proto__: null, + name, + main, + exports, + imports, + type, + }; + + // Execute JSONParse on demand for improved performance + if (parseExports) { + result.exports = JSONParse(exports); + } + + if (parseImports) { + result.imports = JSONParse(imports); + } + if (manifest === undefined) { + const { getOptionValue } = require('internal/options'); manifest = getOptionValue('--experimental-policy') ? require('internal/process/policy').manifest : null; } if (manifest !== null) { const jsonURL = pathToFileURL(jsonPath); - manifest.assertIntegrity(jsonURL, string); + manifest.assertIntegrity(jsonURL, JSONStringify(result)); } } + cache.set(jsonPath, result); return result; } diff --git a/src/node_errors.h b/src/node_errors.h index b3060e7b41c396..2eb1c4ea2866b2 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -66,6 +66,7 @@ void AppendExceptionLine(Environment* env, V(ERR_ILLEGAL_CONSTRUCTOR, Error) \ V(ERR_INVALID_ADDRESS, Error) \ V(ERR_INVALID_ARG_VALUE, TypeError) \ + V(ERR_INVALID_PACKAGE_CONFIG, SyntaxError) \ V(ERR_OSSL_EVP_INVALID_DIGEST, Error) \ V(ERR_INVALID_ARG_TYPE, TypeError) \ V(ERR_INVALID_OBJECT_DEFINE_PROPERTY, TypeError) \ diff --git a/src/node_file.cc b/src/node_file.cc index 5a92432019dbb1..be1a4b43f68f45 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -22,6 +22,7 @@ #include "aliased_buffer-inl.h" #include "memory_tracker-inl.h" #include "node_buffer.h" +#include "node_errors.h" #include "node_external_reference.h" #include "node_file-inl.h" #include "node_process-inl.h" @@ -32,8 +33,11 @@ #include "tracing/trace_event.h" #include "req_wrap-inl.h" +#include "simdjson.h" +#include "simdutf.h" #include "stream_base-inl.h" #include "string_bytes.h" +#include "v8-primitive.h" #include #include @@ -1079,41 +1083,135 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { } const size_t size = offset - start; - char* p = &chars[start]; - char* pe = &chars[size]; - char* pos[2]; - char** ppos = &pos[0]; - - while (p < pe) { - char c = *p++; - if (c == '\\' && p < pe && *p == '"') p++; - if (c != '"') continue; - *ppos++ = p; - if (ppos < &pos[2]) continue; - ppos = &pos[0]; - - char* s = &pos[0][0]; - char* se = &pos[1][-1]; // Exclude quote. - size_t n = se - s; - - if (n == 4) { - if (0 == memcmp(s, "main", 4)) break; - if (0 == memcmp(s, "name", 4)) break; - if (0 == memcmp(s, "type", 4)) break; - } else if (n == 7) { - if (0 == memcmp(s, "exports", 7)) break; - if (0 == memcmp(s, "imports", 7)) break; + simdjson::ondemand::parser parser; + simdjson::padded_string json_string(chars.data() + start, size); + simdjson::ondemand::document document; + simdjson::ondemand::value ondemand_value; + simdjson::error_code error = parser.iterate(json_string).get(document); + + error = document.get_value().get(ondemand_value); + if (error) { + THROW_ERR_INVALID_PACKAGE_CONFIG(isolate, ("Error parsing " + path.ToString()).c_str()); + return; + } + + auto js_string = [&](std::string_view sv) { + return ToV8Value(env->context(), sv, isolate).ToLocalChecked(); + }; + + bool includes_keys{false}; + Local name = Undefined(isolate); + Local main = Undefined(isolate); + Local exports = Undefined(isolate); + Local imports = Undefined(isolate); + Local type = Undefined(isolate); + bool parse_exports{false}; + bool parse_imports{false}; + + // Check for "name" field + std::string_view name_value{}; + error = ondemand_value["name"].get_string().get(name_value); + if (!error) { + name = js_string(name_value); + includes_keys = true; + } + + // Check for "main" field + std::string_view main_value{}; + error = ondemand_value["main"].get_string().get(main_value); + if (!error) { + main = js_string(main_value); + includes_keys = true; + } + + // Check for "exports" field + simdjson::ondemand::json_type exports_json_type; + error = ondemand_value["exports"].type().get(exports_json_type); + if (!error) { + std::string_view exports_value; + switch (exports_json_type) { + case simdjson::ondemand::json_type::object: { + simdjson::ondemand::object exports_object; + if (!ondemand_value["exports"].get_object().get(exports_object)) { + if (!exports_object.raw_json().get(exports_value)) { + exports = js_string(exports_value); + includes_keys = true; + parse_exports = true; + } + } + break; + } + case simdjson::ondemand::json_type::string: { + if (!ondemand_value["exports"].get_string().get(exports_value)) { + exports = js_string(exports_value); + includes_keys = true; + } + break; + } + default: { + // do nothing + } } } + // Check for "imports" field + simdjson::ondemand::json_type imports_json_type; + error = ondemand_value["imports"].type().get(imports_json_type); + if (!error) { + std::string_view imports_value; + switch (imports_json_type) { + case simdjson::ondemand::json_type::object: { + simdjson::ondemand::object imports_object; + if (!ondemand_value["imports"].get_object().get(imports_object)) { + if (!imports_object.raw_json().get(imports_value)) { + imports = js_string(imports_value); + includes_keys = true; + parse_imports = true; + } + } + break; + } + case simdjson::ondemand::json_type::string: { + if (!ondemand_value["imports"].get_string().get(imports_value)) { + imports = js_string(imports_value); + includes_keys = true; + } + break; + } + default: { + // do nothing + } + } + } + + // Check for "type" field + std::string_view type_value = "none"; + error = ondemand_value["type"].get_string().get(type_value); + if (!error) { + // Ignore unknown types for forwards compatibility + if (type_value != "module" && type_value != "commonjs") { + type_value = "none"; + } + includes_keys = true; + } + type = js_string(type_value); + + // No need to optimize for the failed case, since this is highly unlikely. + if (error != simdjson::error_code::NO_SUCH_FIELD && error != simdjson::error_code::SUCCESS) { + THROW_ERR_INVALID_PACKAGE_CONFIG(isolate, ("Error parsing " + path.ToString()).c_str()); + } Local return_value[] = { - String::NewFromUtf8(isolate, - &chars[start], - v8::NewStringType::kNormal, - size).ToLocalChecked(), - Boolean::New(isolate, p < pe ? true : false) + Boolean::New(isolate, includes_keys), + name, + main, + exports, + imports, + type, + Boolean::New(isolate, parse_exports), + Boolean::New(isolate, parse_imports), }; + args.GetReturnValue().Set( Array::New(isolate, return_value, arraysize(return_value))); } diff --git a/test/es-module/test-esm-invalid-pjson.js b/test/es-module/test-esm-invalid-pjson.js deleted file mode 100644 index 9b49f6f1357685..00000000000000 --- a/test/es-module/test-esm-invalid-pjson.js +++ /dev/null @@ -1,29 +0,0 @@ -'use strict'; - -const { checkoutEOL, spawnPromisified } = require('../common'); -const fixtures = require('../common/fixtures.js'); -const assert = require('node:assert'); -const { execPath } = require('node:process'); -const { describe, it } = require('node:test'); - - -describe('ESM: Package.json', { concurrency: true }, () => { - it('should throw on invalid pson', async () => { - const entry = fixtures.path('/es-modules/import-invalid-pjson.mjs'); - const invalidJson = fixtures.path('/node_modules/invalid-pjson/package.json'); - - const { code, signal, stderr } = await spawnPromisified(execPath, [entry]); - - assert.ok( - stderr.includes( - `[ERR_INVALID_PACKAGE_CONFIG]: Invalid package config ${invalidJson} ` + - `while importing "invalid-pjson" from ${entry}. ` + - "Expected ':' after property name in JSON at position " + - `${12 + checkoutEOL.length * 2}` - ), - stderr - ); - assert.strictEqual(code, 1); - assert.strictEqual(signal, null); - }); -}); diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot index a38745e396a8d9..b9cdc66169ba8e 100644 --- a/test/fixtures/errors/force_colors.snapshot +++ b/test/fixtures/errors/force_colors.snapshot @@ -4,10 +4,10 @@ throw new Error('Should include grayed stack trace') Error: Should include grayed stack trace at Object. (/test*force_colors.js:1:7) - at Module._compile (node:internal*modules*cjs*loader:1255:14) - at Module._extensions..js (node:internal*modules*cjs*loader:1309:10) - at Module.load (node:internal*modules*cjs*loader:1113:32) - at Module._load (node:internal*modules*cjs*loader:960:12) + at Module._compile (node:internal*modules*cjs*loader:1228:14) + at Module._extensions..js (node:internal*modules*cjs*loader:1282:10) + at Module.load (node:internal*modules*cjs*loader:1086:32) + at Module._load (node:internal*modules*cjs*loader:933:12)  at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:83:12)  at node:internal*main*run_main_module:23:47 diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js index a0fa0a038990f0..6a915e6190e1e7 100644 --- a/test/parallel/test-module-binding.js +++ b/test/parallel/test-module-binding.js @@ -4,34 +4,35 @@ require('../common'); const fixtures = require('../common/fixtures'); const { internalBinding } = require('internal/test/binding'); const { internalModuleReadJSON } = internalBinding('fs'); -const { readFileSync } = require('fs'); const { strictEqual } = require('assert'); { - const [string, containsKeys] = internalModuleReadJSON('nosuchfile'); - strictEqual(string, undefined); - strictEqual(containsKeys, undefined); + // Non-existing file + const result = internalModuleReadJSON('nosuchfile'); + strictEqual(result.length, 0); } { - const [string, containsKeys] = - internalModuleReadJSON(fixtures.path('empty.txt')); - strictEqual(string, ''); - strictEqual(containsKeys, false); + // Invalid JSON + const result = internalModuleReadJSON(fixtures.path('empty.txt')); + strictEqual(result.length, 0); } { - const [string, containsKeys] = - internalModuleReadJSON(fixtures.path('empty.txt')); - strictEqual(string, ''); - strictEqual(containsKeys, false); -} -{ - const [string, containsKeys] = - internalModuleReadJSON(fixtures.path('empty-with-bom.txt')); - strictEqual(string, ''); - strictEqual(containsKeys, false); + const result = internalModuleReadJSON(fixtures.path('empty-with-bom.txt')); + strictEqual(result.length, 0); } { const filename = fixtures.path('require-bin/package.json'); - const [string, containsKeys] = internalModuleReadJSON(filename); - strictEqual(string, readFileSync(filename, 'utf8')); - strictEqual(containsKeys, true); + const { + 0: includesKeys, + 1: name, + 2: main, + 3: exports, + 4: imports, + 5: type, + } = internalModuleReadJSON(filename); + strictEqual(includesKeys, true); + strictEqual(name, 'req'); + strictEqual(main, './lib/req.js'); + strictEqual(exports, undefined); + strictEqual(imports, undefined); + strictEqual(type, 'none'); }