module: speed up package.json parsing #15767

bnoordhuis · 2017-10-04T11:46:52Z

The first commit is a bug fix and should be back-ported to the release branches but I'm including it here because the other commits build on top of it.

From the second commit log:

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

That 25% is going to depend on how many package.json files have a "main" property. With another project, only a handful out of ~250 did not have one.

CI: https://ci.nodejs.org/job/node-test-pull-request/10401/

Trott · 2017-10-04T16:16:16Z

Thoughts on semver-ness?

bnoordhuis · 2017-10-04T16:52:30Z

I'm inclined to say semver-patch. The change in behavior only affects applications that don't start up in the first place.

mscdex · 2017-10-04T18:37:30Z

lib/module.js

@@ -112,6 +112,9 @@ function readPackage(requestPath) {
    return false;
  }

+  if (json === '')


Does V8 make this and json.length === 0 equivalent instruction-wise?

s.length === 0 is probably a bit slower. The empty string is a singleton so s === '' is basically a pointer comparison, whereas s.length needs to validate s first (although I expect it will be a fast path protected by a guard once optimized.)

guybedford · 2017-10-04T19:14:20Z

On the example project with ~250 dependencies and only a handful without a main, how did the performance copmare? Just wondering how much the slow-down might be for an application where every package.json file does have a main and how negligible this is.

addaleax · 2017-10-04T19:28:13Z

Just to give a number, of the 596 package.jsons in Node’s tools/ and deps/, 364 contain the string "main", so it’s reasonable to assume that about 40 % of package.jsons in the wild can be skipped this way.

bnoordhuis · 2017-10-05T11:46:50Z

On the example project with ~250 dependencies and only a handful without a main, how did the performance copmare?

It's the same, no statistically significant speed-up or slow-down. The string search is practically free compared to deserializing the JSON object.

One potential improvement is to implement our own special-purpose JSON parser that scans for just the "main" property and ignores everything else - that would help with loading e.g. async, which has a > 50 kB package.json.

That's a lot more work though and I'm not even sure it's a good idea per se.

guybedford · 2017-10-05T14:00:31Z

That may well be interesting to work on in future for additional performance gain.

cjihrig

Nice

mscdex · 2017-10-05T19:18:00Z

Should this PR (except the first commit) be semver-major because of the change in module loading behavior (invalid/malformatted modules may no longer be detected)?

bnoordhuis · 2017-10-05T21:21:25Z

@mscdex My opinion in case you missed it: #15767 (comment)

Fishrock123 · 2017-10-19T18:01:23Z

The first commit is a bug fix and should be back-ported to the release branches but I'm including it here because the other commits build on top of it.

It would be far more ideal to releasers if this was separate from commits that aren't that category.

Such files used to
throw an exception but now they are simply ignored unless they contain a
"main" property.

If that exception bubbled up to user land I do think this (the sum of the 3 commits) is semver-major?

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

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.

bnoordhuis · 2017-11-14T11:02:02Z

Rebase + new CI: https://ci.nodejs.org/job/node-test-pull-request/11421/

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 PR-URL: nodejs#15767 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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. PR-URL: nodejs#15767 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis · 2017-11-15T11:03:44Z

1132ea7...0fdd88a and tagged semver-major because I'm conservative like that.

charmander · 2017-11-15T11:12:46Z

"\u006dain"?

jdalton · 2017-11-16T16:35:51Z

I think this should be revisited. InternalModuleReadFile was more generically useful before and could have been applied to loading more than just json.

However, now with the introduction of this bit it essentially locks it into json only. If that's the case, I think the name InternalModuleReadFile, and the pseudo exposed process.binding("fs").internalModuleReadFile, should either be renamed or a new internal for json-only should be created (maybe that wraps InternalModuleReadFile).

Update:
Raised issue #17076 and PR #17084.

charmander · 2017-11-18T16:24:55Z

Does this fail if "main" is written with \u escapes?

bnoordhuis · 2017-11-18T19:02:57Z

@charmander I guess it would. Can you point to a module that does that?

jdalton · 2017-11-18T19:08:29Z

@bnoordhuis

That 25% is going to depend on how many package.json files have a "main" property. With another project, only a handful out of ~250 did not have one.

Was the perf win, when applicable, primarily from the first commit (the one that didn't touch the c++ helper)? If so I think that could be enough, without having to go deeper into the internals #17084.

bnoordhuis · 2017-11-19T12:48:39Z

@jdalton The second commit avoids ~300 kB of strings being created needlessly when you invoke npm. That's pretty substantial.

jdalton · 2017-11-19T12:57:18Z

@bnoordhuis

The second commit avoids ~300 kB of strings being created needlessly when you invoke npm. That's pretty substantial.

What's that mean for execution time? (e.g. does it move from 25% to 30% improvement?)

What I'm getting at is, that while it is nice, it's not a guaranteed perf win since it's super scenario specific (npm init creates package.json files with a "main" field) and also diminishes the usefulness from the helper (moved from generic read file fast path to only json). So it's a step too far in the name of a might-be optimization.

bnoordhuis · 2017-11-19T13:10:59Z

It cuts down the number and total size of package.json strings by about 35%. To put that in perspective: even a 1% or 2% improvement would have been well worth it, every bit helps.

If you don't think it's a worthwhile optimization, the onus is on you to prove it with numbers (but you'd be wasting your time because it is.)

jdalton · 2017-11-19T13:19:58Z

To put that in perspective: even a 1% or 2% improvement would have been well worth it, every bit helps.

In the past, improvements like using InternalReadFile for more than just json, which I'm sure would fall into that category or better, were held off on because of potential breakage.

If you don't think it's a worthwhile optimization, the onus is on you to prove it with numbers

Wouldn't the responsibility be to prove the perf wins before introducing something that requires a semver major? I'm all for optimizations, but I'd like to have a more clear picture on what we're giving up usability and potential user code breakage (semver major) for. I don't think asking for the percentage wins of the second commit is unreasonable.

So again, what's that mean for execution time? (e.g. does it move from 25% to 30% improvement?)

bnoordhuis · 2017-11-19T13:31:10Z

The semver-major tag is just me being ultra-conservative (and also about giving people an incentive to upgrade when v10 comes out.)

jdalton · 2017-11-19T13:38:01Z

@bnoordhuis
Instead of locking InternalReadFile down to just json what are your thoughts on creating a InternalReadJON and using InternalReadFile for more than just json (so loading modules). Do you have a wag on what, if any, the improvements would be from that?

charmander · 2017-11-19T13:56:28Z

@bnoordhuis I can’t, but is it maybe worth also checking for \u006 to avoid making that exception?

bnoordhuis · 2017-11-20T18:32:38Z

@jdalton See #4679, my original PR did that but it caused regressions on Windows and I never quite figured out why.

@charmander I suppose so, I'll think about it. I don't mind doing that if it's necessary but neither do I like adding complexity and overhead for what is most likely a non-issue.

jdalton · 2017-11-20T21:17:43Z

@bnoordhuis might be worth trying again (with today's code base). It looks like there was other Jenkins weirdness back then too.

TimothyGu · 2017-11-20T23:45:04Z

@charmander I think it's mostly a non-issue as npm serializes and reserializes package.json during installation, which replaces \u006d with m. I don't know if Yarn does that, however.

charmander · 2017-11-20T23:58:38Z

It’s a small amount of code complexity and overhead in exchange for replacing “works for almost all valid JSON representations” and “mostly a non-issue” with “works for all JSON representations” and “is a non-issue”. Is the certainty worth having?

bnoordhuis · 2017-11-21T10:50:00Z

Not if it's dead code. Then it's just a place for bugs to hide.

(I don't subscribe to Postel's maxim in case you wondered.)

charmander · 2017-11-21T15:06:20Z

I don't subscribe to Postel's maxim in case you wondered.

That doesn’t really apply here – JSON is JSON. I’ll open a PR.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 4, 2017

addaleax approved these changes Oct 4, 2017

View reviewed changes

mscdex reviewed Oct 4, 2017

View reviewed changes

cjihrig approved these changes Oct 5, 2017

View reviewed changes

jasnell approved these changes Oct 20, 2017

View reviewed changes

bnoordhuis mentioned this pull request Nov 7, 2017

src: fix UB in InternalModuleReadFile() #16871

Merged

bnoordhuis added 2 commits November 14, 2017 11:59

module: speed up package.json parsing more

b46622e

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.

bnoordhuis force-pushed the module-loading branch from 9dae2b3 to b46622e Compare November 14, 2017 11:01

bnoordhuis closed this Nov 15, 2017

bnoordhuis force-pushed the module-loading branch from b46622e to 0fdd88a Compare November 15, 2017 11:02

bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 15, 2017

This was referenced Nov 16, 2017

Rename InternalModuleReadFile and friends. #17076

Closed

Rename internalModuleReadFile to internalModuleReadJSON. #17084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: speed up package.json parsing #15767

module: speed up package.json parsing #15767

bnoordhuis commented Oct 4, 2017 •

edited

Loading

Trott commented Oct 4, 2017

bnoordhuis commented Oct 4, 2017

mscdex Oct 4, 2017

bnoordhuis Oct 5, 2017

guybedford commented Oct 4, 2017

addaleax commented Oct 4, 2017

bnoordhuis commented Oct 5, 2017

guybedford commented Oct 5, 2017

cjihrig left a comment

mscdex commented Oct 5, 2017 •

edited

Loading

bnoordhuis commented Oct 5, 2017

Fishrock123 commented Oct 19, 2017

bnoordhuis commented Nov 14, 2017

bnoordhuis commented Nov 15, 2017

charmander commented Nov 15, 2017

jdalton commented Nov 16, 2017 •

edited

Loading

charmander commented Nov 18, 2017

bnoordhuis commented Nov 18, 2017

jdalton commented Nov 18, 2017 •

edited

Loading

bnoordhuis commented Nov 19, 2017

jdalton commented Nov 19, 2017 •

edited

Loading

bnoordhuis commented Nov 19, 2017

jdalton commented Nov 19, 2017 •

edited

Loading

bnoordhuis commented Nov 19, 2017

jdalton commented Nov 19, 2017 •

edited

Loading

charmander commented Nov 19, 2017

bnoordhuis commented Nov 20, 2017

jdalton commented Nov 20, 2017

TimothyGu commented Nov 20, 2017 •

edited

Loading

charmander commented Nov 20, 2017

bnoordhuis commented Nov 21, 2017

charmander commented Nov 21, 2017

module: speed up package.json parsing #15767

module: speed up package.json parsing #15767

Conversation

bnoordhuis commented Oct 4, 2017 • edited Loading

Trott commented Oct 4, 2017

bnoordhuis commented Oct 4, 2017

mscdex Oct 4, 2017

Choose a reason for hiding this comment

bnoordhuis Oct 5, 2017

Choose a reason for hiding this comment

guybedford commented Oct 4, 2017

addaleax commented Oct 4, 2017

bnoordhuis commented Oct 5, 2017

guybedford commented Oct 5, 2017

cjihrig left a comment

Choose a reason for hiding this comment

mscdex commented Oct 5, 2017 • edited Loading

bnoordhuis commented Oct 5, 2017

Fishrock123 commented Oct 19, 2017

bnoordhuis commented Nov 14, 2017

bnoordhuis commented Nov 15, 2017

charmander commented Nov 15, 2017

jdalton commented Nov 16, 2017 • edited Loading

charmander commented Nov 18, 2017

bnoordhuis commented Nov 18, 2017

jdalton commented Nov 18, 2017 • edited Loading

bnoordhuis commented Nov 19, 2017

jdalton commented Nov 19, 2017 • edited Loading

bnoordhuis commented Nov 19, 2017

jdalton commented Nov 19, 2017 • edited Loading

bnoordhuis commented Nov 19, 2017

jdalton commented Nov 19, 2017 • edited Loading

charmander commented Nov 19, 2017

bnoordhuis commented Nov 20, 2017

jdalton commented Nov 20, 2017

TimothyGu commented Nov 20, 2017 • edited Loading

charmander commented Nov 20, 2017

bnoordhuis commented Nov 21, 2017

charmander commented Nov 21, 2017

bnoordhuis commented Oct 4, 2017 •

edited

Loading

mscdex commented Oct 5, 2017 •

edited

Loading

jdalton commented Nov 16, 2017 •

edited

Loading

jdalton commented Nov 18, 2017 •

edited

Loading

jdalton commented Nov 19, 2017 •

edited

Loading

jdalton commented Nov 19, 2017 •

edited

Loading

jdalton commented Nov 19, 2017 •

edited

Loading

TimothyGu commented Nov 20, 2017 •

edited

Loading