From 2e4aae4691b1de9dac34b227f1e3db9573ea6ce1 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 9 Nov 2017 13:26:46 +0100 Subject: [PATCH 1/2] module: speed up package.json parsing If the package.json does not contain the string '"main"', skip parsing it to JSON. Note that this changes the behavior of the module loader in the presence of package.json files that don't contain legal JSON. Such files used to throw an exception but now they are simply ignored unless they contain a "main" property. To me, that seems like a good trade-off: I observe a 25% reduction in start-up time on a medium-sized application[0]. [0] https://github.com/strongloop/sls-sample-app --- lib/module.js | 3 +++ test/fixtures/packages/invalid/index.js | 1 - test/fixtures/packages/invalid/package.json | 1 - test/sequential/test-module-loading.js | 8 -------- 4 files changed, 3 insertions(+), 10 deletions(-) delete mode 100644 test/fixtures/packages/invalid/index.js delete mode 100644 test/fixtures/packages/invalid/package.json diff --git a/lib/module.js b/lib/module.js index e418a1a3e08440..b9de9c3edfb782 100644 --- a/lib/module.js +++ b/lib/module.js @@ -120,6 +120,9 @@ function readPackage(requestPath) { return false; } + if (!/"main"/.test(json)) + return packageMainCache[requestPath] = undefined; + try { var pkg = packageMainCache[requestPath] = JSON.parse(json).main; } catch (e) { diff --git a/test/fixtures/packages/invalid/index.js b/test/fixtures/packages/invalid/index.js deleted file mode 100644 index 014fa39dc365d1..00000000000000 --- a/test/fixtures/packages/invalid/index.js +++ /dev/null @@ -1 +0,0 @@ -exports.ok = 'ok'; diff --git a/test/fixtures/packages/invalid/package.json b/test/fixtures/packages/invalid/package.json deleted file mode 100644 index 004e1e20324524..00000000000000 --- a/test/fixtures/packages/invalid/package.json +++ /dev/null @@ -1 +0,0 @@ -{,} diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js index 47916d352d729a..790331a37b9d80 100644 --- a/test/sequential/test-module-loading.js +++ b/test/sequential/test-module-loading.js @@ -100,14 +100,6 @@ const d2 = require('../fixtures/b/d'); assert.notStrictEqual(threeFolder, three); } -console.error('test package.json require() loading'); -assert.throws( - function() { - require('../fixtures/packages/invalid'); - }, - /^SyntaxError: Error parsing .+: Unexpected token , in JSON at position 1$/ -); - assert.strictEqual(require('../fixtures/packages/index').ok, 'ok', 'Failed loading package'); assert.strictEqual(require('../fixtures/packages/main').ok, 'ok', From b46622e26e1037138aa5a15a0baa2a2d0673b64e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 9 Nov 2017 13:26:47 +0100 Subject: [PATCH 2/2] module: speed up package.json parsing more Move the logic from the previous commit to C++ land in order to avoid creating a new string when we know we won't parse it anyway. --- lib/module.js | 2 +- src/node_file.cc | 8 +++++--- src/string_search.h | 11 ++++++++++- test/parallel/test-module-binding.js | 5 +++++ 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/module.js b/lib/module.js index b9de9c3edfb782..cc8d5097bb83b2 100644 --- a/lib/module.js +++ b/lib/module.js @@ -120,7 +120,7 @@ function readPackage(requestPath) { return false; } - if (!/"main"/.test(json)) + if (json === '') return packageMainCache[requestPath] = undefined; try { diff --git a/src/node_file.cc b/src/node_file.cc index 76ab7f9f337829..216b60485663b6 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -25,6 +25,7 @@ #include "req-wrap-inl.h" #include "string_bytes.h" +#include "string_search.h" #include #include @@ -466,8 +467,9 @@ void FillStatsArray(double* fields, const uv_stat_t* s) { } // Used to speed up module loading. Returns the contents of the file as -// a string or undefined when the file cannot be opened. The speedup -// comes from not creating Error objects on failure. +// a string or undefined when the file cannot be opened. Returns an empty +// string when the file does not contain the substring '"main"' because that +// is the property we care about. static void InternalModuleReadFile(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); uv_loop_t* loop = env->event_loop(); @@ -516,7 +518,7 @@ static void InternalModuleReadFile(const FunctionCallbackInfo& args) { } const size_t size = offset - start; - if (size == 0) { + if (size == 0 || size == SearchString(&chars[start], size, "\"main\"")) { args.GetReturnValue().SetEmptyString(); } else { Local chars_string = diff --git a/src/string_search.h b/src/string_search.h index 6e76a5b8f413ea..51f72062a99466 100644 --- a/src/string_search.h +++ b/src/string_search.h @@ -638,6 +638,7 @@ size_t SearchString(const Char* haystack, size_t needle_length, size_t start_index, bool is_forward) { + if (haystack_length < needle_length) return haystack_length; // To do a reverse search (lastIndexOf instead of indexOf) without redundant // code, create two vectors that are reversed views into the input strings. // For example, v_needle[0] would return the *last* character of the needle. @@ -646,7 +647,6 @@ size_t SearchString(const Char* haystack, needle, needle_length, is_forward); Vector v_haystack = Vector( haystack, haystack_length, is_forward); - CHECK(haystack_length >= needle_length); size_t diff = haystack_length - needle_length; size_t relative_start_index; if (is_forward) { @@ -664,6 +664,15 @@ size_t SearchString(const Char* haystack, } return is_forward ? pos : (haystack_length - needle_length - pos); } + +template +size_t SearchString(const char* haystack, size_t haystack_length, + const char (&needle)[N]) { + return SearchString( + reinterpret_cast(haystack), haystack_length, + reinterpret_cast(needle), N - 1, 0, true); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js index ba11c3e47ec646..f5c2a794b110c7 100644 --- a/test/parallel/test-module-binding.js +++ b/test/parallel/test-module-binding.js @@ -2,8 +2,13 @@ require('../common'); const fixtures = require('../common/fixtures'); const { internalModuleReadFile } = process.binding('fs'); +const { readFileSync } = require('fs'); const { strictEqual } = require('assert'); strictEqual(internalModuleReadFile('nosuchfile'), undefined); strictEqual(internalModuleReadFile(fixtures.path('empty.txt')), ''); strictEqual(internalModuleReadFile(fixtures.path('empty-with-bom.txt')), ''); +{ + const filename = fixtures.path('require-bin/package.json'); + strictEqual(internalModuleReadFile(filename), readFileSync(filename, 'utf8')); +}