module: ignore package.json/ directories by bmeck · Pull Request #18261 · nodejs/node

bmeck · 2018-01-19T21:07:30Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

module

jdalton · 2018-01-19T21:15:16Z

Could this be handled at the C++ level of the InternalModuleReadFile/InternalReadModuleJSON methods instead? That way the guard isn't needed to be added to each potential use of the internal method.

bmeck · 2018-01-19T21:18:03Z

@jdalton could, but then it won't be in the stat cache.

jdalton · 2018-01-19T21:20:17Z

Is there a way to share a cache between C++ helpers and external...
kinda like the statValues typed array?

bmeck · 2018-01-19T21:34:15Z

@jdalton the statValues approach has a known size, the stat cache is a growable Map. I don't think the same approach can be used.

jdalton · 2018-01-19T21:42:11Z

Ah, okay.

guybedford · 2018-01-19T21:54:30Z

If internalModuleReadJSON returned an empty string in this case, surely that would be cached as an empty main resulting in the same behaviour, whilst avoiding round-tripping latency?

richardlau · 2018-01-20T03:15:52Z

+
+try {
+  require(fixtures.path('module-package-json-directory'));
+  process.exit(1);


Would it be better to assert.fail here instead? I'm thinking from the point of view of how it might look in the CI if the test failed here.

Can't we just use assert.throws()?

it would just be caught by the catch below.

TimothyGu

LGTM with @richardlau's comment fixed.

bnoordhuis

This shouldn't land as-is. A lot of time and effort has been spent on making this particular code path fast.

bnoordhuis · 2018-01-20T11:28:51Z

    return entry;

  const jsonPath = path.resolve(requestPath, 'package.json');
+  const rc = stat(jsonPath);


Don't introduce extra system calls that are practically never necessary. Handle it inside InternalModuleReadJSON(), it's the CHECK_GE(numchars, 0) in that function.

edit: since it's easier to just write it than try and explain it: #18270

bmeck · 2018-01-20T14:50:54Z

@bnoordhuis your requests have been upstreamed to this PR

bmeck · 2018-01-20T14:51:57Z

@guybedford changed to use ""

@jdalton moved to InternalModuleReadJSON

bmeck · 2018-01-20T15:27:42Z

there was a bug in how packageMainCache was checking for entries if it InternalModuleReadJSON returned an empty string since it only checked falsey values. This has been fixed but I'd like a rereview due to that change.

bnoordhuis · 2018-01-20T15:38:17Z

@bmeck Taking my code and merging that as your own without attribution or even so much as a thank you... that's not a nice thing to do.

bmeck · 2018-01-20T15:49:32Z

@bnoordhuis I'd agree but have similar thoughts on repeated interactions with you. I'll add your attribution, you didn't seem to have any attribution or care about my efforts on your PR either though.

Author: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis · 2018-01-20T15:55:59Z

you didn't seem to have any attribution [..] on your PR

That's because there's nothing to attribute, I didn't take code or ideas from this PR.

bmeck · 2018-01-20T15:57:23Z

@bnoordhuis your statements reinforce my feelings being similar to yours at being upset at the other person.

bmeck · 2018-01-27T17:03:22Z

moving conversation to @bnoordhuis 's PR after having problems with differences in internal consistency choices. it ended up expanding the decision making process and can be discussed there.

jdalton · 2018-01-27T20:18:08Z

Moved to #18270.

bmeck · 2018-01-29T15:53:46Z

Reopened for comparative review from @nodejs/tsc .

jasnell · 2018-01-29T18:34:00Z

Actual benchmark run so we're not just guessing about potential perf impact: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/109/console

jasnell · 2018-01-29T20:30:33Z

At least for the module-loader microbenchmarks, there is no appreciable impact ...

                                                                        confidence improvement accuracy (*)    (**)   (***)
 module/module-loader.js useCache="false" fullPath="false" thousands=50                -1.83 %       ±5.23%  ±6.96%  ±9.05%
 module/module-loader.js useCache="false" fullPath="true" thousands=50                  1.49 %       ±4.89%  ±6.51%  ±8.48%
 module/module-loader.js useCache="true" fullPath="false" thousands=50                  0.31 %      ±10.37% ±13.80% ±17.98%
 module/module-loader.js useCache="true" fullPath="true" thousands=50                   1.14 %      ±10.69% ±14.23% ±18.54%

Not sure how much (if at all, that benchmark exercises this code, however)

bnoordhuis · 2018-01-29T20:45:19Z

FWIW, I benchmark using one of the loopback sample apps (loads ~5,000 files and did something like 250K stat calls at first, IIRC) but any biggish real-world application should do. Warming up the disk cache with a few trial runs is a good idea.

jasnell · 2018-01-29T21:43:08Z

Ok, do you have any numbers from the loopback sample that show the changes in this PR being a problem?

mcollina

LGTM

outdated

targos · 2018-02-06T08:14:14Z

I am not against this PR but I would prefer to have the cache changes in a separate one

Trott · 2018-02-08T05:07:55Z

Closed in favor of #18270. (If that's not the right thing to do, by all means, re-open!).

module: ignore package.json/ directories

fec4a5d

Fixes: nodejs#8307

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Jan 19, 2018

addaleax approved these changes Jan 19, 2018

View reviewed changes

richardlau reviewed Jan 20, 2018

View reviewed changes

TimothyGu approved these changes Jan 20, 2018

View reviewed changes

bnoordhuis requested changes Jan 20, 2018

View reviewed changes

bnoordhuis mentioned this pull request Jan 20, 2018

src: don't abort when package.json is a directory #18270

Closed

targos previously approved these changes Jan 20, 2018

View reviewed changes

bnoordhuis and others added 3 commits January 20, 2018 09:50

bnoordhuis nits

8879cc2

Author: Ben Noordhuis <info@bnoordhuis.nl>

fix bug on empty main

4d668cb

fixup extra close_req

bc7bdec

bmeck force-pushed the issue-8307 branch from ad50428 to bc7bdec Compare January 20, 2018 15:51

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

bmeck closed this Jan 27, 2018

bmeck reopened this Jan 29, 2018

jasnell approved these changes Jan 29, 2018

View reviewed changes

mcollina approved these changes Feb 6, 2018

View reviewed changes

Trott closed this Feb 8, 2018

Uh oh!

Conversation

bmeck commented Jan 19, 2018

Checklist

Affected core subsystem(s)

Uh oh!

jdalton commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeck commented Jan 19, 2018

Uh oh!

jdalton commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeck commented Jan 19, 2018

Uh oh!

jdalton commented Jan 19, 2018

Uh oh!

guybedford commented Jan 19, 2018

Uh oh!

richardlau Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

targos Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

bmeck Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmeck Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

bmeck commented Jan 20, 2018

Uh oh!

bmeck commented Jan 20, 2018

Uh oh!

bmeck commented Jan 20, 2018

Uh oh!

bnoordhuis commented Jan 20, 2018

Uh oh!

bmeck commented Jan 20, 2018

Uh oh!

bnoordhuis commented Jan 20, 2018

Uh oh!

bmeck commented Jan 20, 2018

Uh oh!

bmeck commented Jan 27, 2018

Uh oh!

jdalton commented Jan 27, 2018

Uh oh!

bmeck commented Jan 29, 2018

Uh oh!

jasnell commented Jan 29, 2018

Uh oh!

jasnell commented Jan 29, 2018

Uh oh!

bnoordhuis commented Jan 29, 2018

Uh oh!

jasnell commented Jan 29, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Feb 6, 2018

Uh oh!

Trott commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

jdalton commented Jan 19, 2018 •

edited

Loading

jdalton commented Jan 19, 2018 •

edited

Loading

bnoordhuis Jan 20, 2018 •

edited

Loading