cache module with same version and same registry

[hustxiaoc]

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]

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

[hustxiaoc]

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

[bnoordhuis]

Can you use English please?

[ChALkeR]

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.

[yaniswang]

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]

Maybe enabled by cli parameter is better?

node --module-dedupe

[bnoordhuis]

Where does the _resolved property come from?

[yaniswang]

{
  "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": "[email protected]"
  },
  "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": "[email protected]",
  "_shasum": "9df7e52fb2a0cb0fb89058ee80c3104225f37e1d",
  "_from": "[email protected]",
  "_npmVersion": "2.1.12",
  "_nodeVersion": "0.11.14",
  "_npmUser": {
    "name": "zhiyelee",
    "email": "[email protected]"
  },
  "maintainers": [
    {
      "name": "tjholowaychuk",
      "email": "[email protected]"
    },
    {
      "name": "somekittens",
      "email": "[email protected]"
    },
    {
      "name": "zhiyelee",
      "email": "[email protected]"
    },
    {
      "name": "thethomaseffect",
      "email": "[email protected]"
    }
  ],
  "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!"
}

[bnoordhuis]

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

[yaniswang]

Or replace to shasum?

[bnoordhuis]

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.

[hustxiaoc]

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?

[fengmk2]

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

[yaniswang]

Or tarball

[ChALkeR]

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]

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]

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

[hustxiaoc]

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

[madbence]

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]

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]

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

[madbence]

@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]

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

[ChALkeR]

@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]

@ChALkeR ？

[fengmk2]

@hustxiaoc @ChALkeR meaning heapTotal not 15.9%↓

heapTotal real is 1.59%↓

(63907840 - 62887936) / 63907840 * 100
1.595898093254286

[hustxiaoc]

@fengmk2 @ChALkeR got it! 😊

[yaniswang]

My case.

[vkurchatkin]

Closing since module resolving algorithm is locked