src: improve package.json reader performance

nodejs · May 23, 2023 · 05ed646 · 05ed646
1 parent e66cc1c
commit 05ed646
Show file tree

Hide file tree

Showing 7 changed files with 144 additions and 166 deletions.
diff --git 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.<ext>
 //   -> a/index.<ext>
 
-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
@@ -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<string, unknown>} Exports
@@ -42,9 +34,10 @@ 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 = {
+  const packageJSON = packageJsonReader.read(path);
+
+  if (packageJSON === undefined) {
+    const json = {
       pjsonPath: path,
       exists: false,
       main: undefined,
@@ -53,48 +46,19 @@ function getPackageConfig(path, specifier, base) {
       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,
-    );
+    packageJSONCache.set(path, json);
+    return json;
   }
 
-  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';
+  if (packageJSON.type !== 'module' && packageJSON.type !== 'commonjs') {
+    packageJSON.type = 'none';
   }
 
-  const packageConfig = {
-    pjsonPath: path,
-    exists: true,
-    main,
-    name,
-    type,
-    exports,
-    imports,
-  };
-  packageJSONCache.set(path, packageConfig);
-  return packageConfig;
+  packageJSON.pjsonPath = path;
+  packageJSON.exists = true;
+  packageJSONCache.set(path, packageJSON);
+  return packageJSON;
 }
 
 

diff --git 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,53 @@ const cache = new SafeMap();
 let manifest;
 
 /**
- *
+ * Returns undefined for all failure cases.
  * @param {string} jsonPath
  */
 function read(jsonPath) {
   if (cache.has(jsonPath)) {
     return cache.get(jsonPath);
   }
 
-  const { 0: string, 1: containsKeys } = internalModuleReadJSON(
+  const {
+    0: includes_keys,
+    1: name,
+    2: main,
+    3: exports,
+    4: imports,
+    5: type,
+  } = internalModuleReadJSON(
     toNamespacedPath(jsonPath),
   );
-  const result = { string, containsKeys };
+
+  let result;
+
+  if (includes_keys !== undefined) {
+    result = { name, main, exports, imports, type, includes_keys };
+
+    // Execute JSONParse on demand for improving performance
+    if (exports) {
+      result.exports = JSONParse(exports);
+    }
+
+    if (imports) {
+      result.imports = JSONParse(imports);
+    }
+  }
+
   const { getOptionValue } = require('internal/options');
-  if (string !== undefined) {
+  if (name !== undefined) {
     if (manifest === undefined) {
       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_file.cc b/src/node_file.cc
@@ -32,8 +32,10 @@
 #include "tracing/trace_event.h"
 
 #include "req_wrap-inl.h"
+#include "simdjson.h"
 #include "stream_base-inl.h"
 #include "string_bytes.h"
+#include "v8-primitive.h"
 
 #include <fcntl.h>
 #include <sys/types.h>
@@ -1079,41 +1081,77 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& 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::object obj;
+  auto error = parser.iterate(json_string).get(document);
+
+  if (error || document.get_object().get(obj)) {
+    args.GetReturnValue().Set(Array::New(isolate));
+    return;
+  }
+
+  auto js_string = [&](std::string_view sv) {
+    return ToV8Value(env->context(), sv, isolate).ToLocalChecked();
+  };
+
+  bool includes_keys{false};
+  Local<Value> name = Undefined(isolate);
+  Local<Value> main = Undefined(isolate);
+  Local<Value> exports = Undefined(isolate);
+  Local<Value> imports = Undefined(isolate);
+  Local<Value> type = Undefined(isolate);
+
+  // Check for "name" field
+  std::string_view name_value{};
+  if (!obj["name"].get_string().get(name_value)) {
+    name = js_string(name_value);
+    includes_keys = true;
+  }
+
+  // Check for "main" field
+  std::string_view main_value{};
+  if (!obj["main"].get_string().get(main_value)) {
+    main = js_string(main_value);
+    includes_keys = true;
+  }
+
+  simdjson::ondemand::object subobject;
+  std::string_view subobject_value{};
+
+  // Check for "exports" field
+  if (!obj["exports"].get_object().get(subobject)) {
+    if (!subobject.raw_json().get(subobject_value)) {
+      exports = js_string(subobject_value);
+      includes_keys = true;
     }
   }
 
+  // Check for "imports" field
+  if (!obj["imports"].get_object().get(subobject)) {
+    if (!subobject.raw_json().get(subobject_value)) {
+      imports = js_string(subobject_value);
+      includes_keys = true;
+    }
+  }
+
+  // Check for "type" field
+  std::string_view type_value{};
+  if (!obj["type"].get(type_value)) {
+    type = js_string(type_value);
+    includes_keys = true;
+  }
 
   Local<Value> 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,
   };
+
   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
diff --git 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.<anonymous> [90m(/[39mtest*force_colors.js:1:7[90m)[39m
-[90m    at Module._compile (node:internal*modules*cjs*loader:1255:14)[39m
-[90m    at Module._extensions..js (node:internal*modules*cjs*loader:1309:10)[39m
-[90m    at Module.load (node:internal*modules*cjs*loader:1113:32)[39m
-[90m    at Module._load (node:internal*modules*cjs*loader:960:12)[39m
+[90m    at Module._compile (node:internal*modules*cjs*loader:1228:14)[39m
+[90m    at Module._extensions..js (node:internal*modules*cjs*loader:1282:10)[39m
+[90m    at Module.load (node:internal*modules*cjs*loader:1086:32)[39m
+[90m    at Module._load (node:internal*modules*cjs*loader:933:12)[39m
 [90m    at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:83:12)[39m
 [90m    at node:internal*main*run_main_module:23:47[39m