Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cache module with same version and same registry #3289

Closed
wants to merge 1 commit into from
Closed

cache module with same version and same registry #3289

wants to merge 1 commit into from

Conversation

hustxiaoc
Copy link

related pr nodejs/node-v0.x-archive#8617

in my project, I start a single node process and here are the memoryUsage.

without cache
{ rss: 84873216, heapTotal: 63907840, heapUsed: 38901048 }

and with cache
{ rss: 72458240, heapTotal: 62887936, heapUsed: 33719160 }

rss 14.6%↓ , heapTotal 1.59%↓, heapUsed 13.3%↓

JacksonTian
JacksonTian reviewed Oct 9, 2015
View reviewed changes
lib/module.js
@@ -76,7 +76,7 @@ function readPackage(requestPath) {
}

try {
var pkg = packageMainCache[requestPath] = JSON.parse(json).main;
var pkg = packageMainCache[requestPath] = JSON.parse(json);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个会导致内存中带有很多无关信息。

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

相比于多次加载模块使用的内存少多了

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use English please?

@ChALkeR
Copy link
Member

ChALkeR commented Oct 9, 2015

This is going to be solved by npm3, isn't it? I am not sure that this is a good idea to do something like that on node.js side, it could break things.

@ChALkeR ChALkeR added semver-major PRs that contain breaking changes and should be released in the next major version. module Issues and PRs related to the module subsystem. labels Oct 9, 2015
@yaniswang
Copy link

It's good idea, it can reduce memory.

Modules:

a(1.0.0) -> x(1.0.0)
b(1.0.0) -> x(2.0.0)
c(1.0.0) -> x(1.0.0)
d(1.0.0) -> x(2.0.0)

Memory:

a(1.0.0)
b(1.0.0)
c(1.0.0)
d(1.0.0)
x(1.0.0)
x(2.0.0)
x(1.0.0)
x(2.0.0)

After this idea:

a(1.0.0)
b(1.0.0)
c(1.0.0)
d(1.0.0)
x(1.0.0)
x(2.0.0)

It can reduce 2 modules.

@yaniswang
Copy link

Maybe enabled by cli parameter is better?

node --module-dedupe

bnoordhuis
bnoordhuis reviewed Oct 9, 2015
View reviewed changes
lib/module.js
if (!pkg) return false;
if (!(pkg && pkg.main)) return false;

var resolved = pkg['_resolved'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the _resolved property come from?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

{
  "name": "commander",
  "version": "2.6.0",
  "description": "the complete solution for node.js command-line programs",
  "keywords": [
    "command",
    "option",
    "parser",
    "prompt"
  ],
  "author": {
    "name": "TJ Holowaychuk",
    "email": "tj@vision-media.ca"
  },
  "license": "MIT",
  "repository": {
    "type": "git",
    "url": "https://github.com/tj/commander.js.git"
  },
  "devDependencies": {
    "should": ">= 0.0.1"
  },
  "scripts": {
    "test": "make test"
  },
  "main": "index",
  "engines": {
    "node": ">= 0.6.x"
  },
  "files": [
    "index.js"
  ],
  "gitHead": "c6807fd154dd3b7ce8756f141f8d3acfcc74be60",
  "bugs": {
    "url": "https://github.com/tj/commander.js/issues"
  },
  "homepage": "https://github.com/tj/commander.js",
  "_id": "commander@2.6.0",
  "_shasum": "9df7e52fb2a0cb0fb89058ee80c3104225f37e1d",
  "_from": "commander@2.6.0",
  "_npmVersion": "2.1.12",
  "_nodeVersion": "0.11.14",
  "_npmUser": {
    "name": "zhiyelee",
    "email": "zhiyelee@gmail.com"
  },
  "maintainers": [
    {
      "name": "tjholowaychuk",
      "email": "tj@vision-media.ca"
    },
    {
      "name": "somekittens",
      "email": "rkoutnik@gmail.com"
    },
    {
      "name": "zhiyelee",
      "email": "zhiyelee@gmail.com"
    },
    {
      "name": "thethomaseffect",
      "email": "thethomaseffect@gmail.com"
    }
  ],
  "dist": {
    "shasum": "9df7e52fb2a0cb0fb89058ee80c3104225f37e1d",
    "tarball": "http://registry.npmjs.org/commander/-/commander-2.6.0.tgz"
  },
  "directories": {},
  "_resolved": "http://registry.npmjs.org/commander/-/commander-2.6.0.tgz",
  "readme": "ERROR: No README data found!"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. What if _resolved is not as unique as this patch assumes? Things will break badly, won't they?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or replace to shasum?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the _shasum field? Same issue: what if it's not unique, e.g., because someone accidentally copy-pasted it from one package.json to another?

You could perhaps take the SHA-1 checksum of the whole package.json file but that requires that node is compiled with openssl support. You can add workarounds for the non-openssl case but I don't know, that gets complex awfully fast.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when running npm install , does it not download the source code from the _resolved url ? so the _resolved url got be unique, otherwise things will break badly, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_resolved is private property from npm registry, should not use it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or tarball

@ChALkeR
Copy link
Member

ChALkeR commented Oct 9, 2015

As it is now, it is a semver-major change in the Modules API. Per doc, Modules system is Locked, so this should be automatically rejected.

https://nodejs.org/api/modules.html#modules_modules
https://nodejs.org/api/documentation.html#documentation_stability_index

@bnoordhuis
Copy link
Member

I'm inclined to agree. It looks to me like the fallout potential is non-zero. That would make it an automatic no-go.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 9, 2015

Also, measuring the memory usage savings in % is inapplicable here. It doesn't scale.

@ChALkeR ChALkeR added the memory Issues and PRs related to the memory management or memory footprint. label Oct 9, 2015
@hustxiaoc
Copy link
Author

so what's the best way to solve this problem ?

@madbence
Copy link

madbence commented Oct 9, 2015

afaik npm3 solves this issue by moving deps up in the dep tree as much as possible (there are a few edge-cases where this cannot work of course)

@yaniswang
Copy link

How to move deps up in this scene?

a(1.0.0) -> x(1.0.0)
b(1.0.0) -> x(2.0.0)
c(1.0.0) -> x(1.0.0)
d(1.0.0) -> x(2.0.0)

@fengmk2
Copy link
Contributor

fengmk2 commented Oct 9, 2015

@yaniswang can you use those real npm package names instead of a, b, c, d for examples?

@madbence
Copy link

madbence commented Oct 9, 2015

@yaniswang that's the edge case 😸 (btw nodejs/NG#20 is another approach to handle this)

btw you should create PRs in a and c to upgrade x to 2 😉

@yaniswang
Copy link

image
nodejs/NG#20
Yes, This can solve this problem, but where is module v2.0.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 9, 2015

@hustxiaoc

without cache
{ rss: 84873216, heapTotal: 63907840, heapUsed: 38901048 }

and with cache
{ rss: 72458240, heapTotal: 62887936, heapUsed: 33719160 }

rss 14.6%↓ , heapTotal 15.9%↓, heapUsed 13.3% ↓

Btw, something is wrong with your numbers here.

@hustxiaoc
Copy link
Author

@ChALkeR ？

@fengmk2
Copy link
Contributor

fengmk2 commented Oct 9, 2015

@hustxiaoc @ChALkeR meaning heapTotal not 15.9%↓

heapTotal real is 1.59%↓

(63907840 - 62887936) / 63907840 * 100
1.595898093254286

@hustxiaoc
Copy link
Author

@fengmk2 @ChALkeR got it! 😊

@yaniswang
Copy link

image
My case.

@vkurchatkin
Copy link
Contributor

Closing since module resolving algorithm is locked

This was referenced May 13, 2024
Open
Open
This was referenced May 14, 2024
Open
Open
@aliscco aliscco mentioned this pull request May 14, 2024
Open
@leonardoadame leonardoadame mentioned this pull request May 14, 2024
Open
This was referenced May 14, 2024
Open
Open
Open
This was referenced May 22, 2024
Open
Open
This was referenced May 23, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
memory Issues and PRs related to the memory management or memory footprint. module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hustxiaoc @ChALkeR @yaniswang @bnoordhuis @madbence @fengmk2 @vkurchatkin @JacksonTian