lib,src: split up big builtin scripts #2714

bnoordhuis · 2015-09-06T11:35:29Z

Reduces the body of parsed source code from 210 kb to 80 kb for a null
program (node -e 0) and from 255 kb to 190 kb for a one-liner that
prints some output to the console.

It doesn't seem to have much impact on the size of the generated code
(on my system I see an 8% reduction with --nolazy and 3% without) but
hopefully it speeds up startup on slow systems because there is now
less source code to parse.

Do not merge yet; this is an experiment and for soliciting feedback.

See #2138.

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

Reduces the body of parsed source code from 210 kb to 80 kb for a null program (`node -e 0`) and from 255 kb to 190 kb for a one-liner that prints some output to the console. It doesn't seem to have much impact on the size of the generated code (on my system I see an 8% reduction with --nolazy and 3% without) but hopefully it speeds up startup on slow systems because there is now less source code to parse.

cjihrig · 2015-09-06T13:13:13Z

Looks like this PR is affected by the issue with 617ee32.

ChALkeR · 2015-09-06T14:18:24Z

@cjihrig All the PRs that were published (or rebased) in the last few days need to be rebased now.

bnoordhuis · 2015-09-06T14:39:24Z

Ah, it wasn't like that when I opened the PR this morning. Anyway, rebased and new CI started: https://ci.nodejs.org/job/node-test-pull-request/256/

thefourtheye · 2015-09-06T14:42:21Z

lib/_stream_readable.js

 const Buffer = require('buffer').Buffer;
-const util = require('util');
-const debug = util.debuglog('stream');
+const EE = require('events').EventEmitter;


EventEmitter at the end could be removed along with this change I guess.

bnoordhuis · 2015-09-06T14:52:19Z

make test-npm is failing rather spectacularly. Looks like it hasn't updated to a version of graceful-fs that doesn't monkey-patch fs? That change landed in graceful-fs in June IIRC. /cc @isaacs @zkat

thefourtheye · 2015-09-06T14:56:25Z

@bnoordhuis npm 2.14.2 has graceful-fs ~4.1.2 as its dependency. That has the fix which you proposed.

Fishrock123 · 2015-09-06T15:01:41Z

@bnoordhuis could you send me some make test-npm output?

bnoordhuis · 2015-09-06T15:15:27Z

@Fishrock123 E.g. here:

Error: Cannot find module 'internal/errno_exception'
    at Function.Module._resolveFilename (module.js:339:15)
    at Function.Module._load (module.js:289:25)
    at Module.require (module.js:368:17)
    at require (module.js:387:17)
    at evalmachine.<anonymous>:36:24
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora22/deps/npm/node_modules/node-gyp/node_modules/graceful-fs/fs.js:11:1)
    at Module._compile (module.js:437:26)
    at Object.Module._extensions..js (module.js:455:10)
    at Module.load (module.js:358:32)
    at Function.Module._load (module.js:313:12)

deps/npm/node_modules/node-gyp/node_modules/graceful-fs/fs.js is still monkey-patching things.

bnoordhuis · 2015-09-06T15:16:44Z

I guess the problem is that npm and node-gyp depend on different versions of graceful-fs....

bnoordhuis · 2015-09-06T15:28:29Z

@thefourtheye Thanks for filing that node-gyp pull request but I don't think that's going to be enough, there's a whole bunch of npm dependencies that depend on outdated versions of graceful-fs:

$ find deps/npm/ -name package.json | xargs grep -E '"graceful-fs"\s*:' | grep -v '4\.1\.2'
deps/npm/node_modules/fs-vacuum/package.json:    "graceful-fs": "^3.0.2",
deps/npm/node_modules/node-gyp/node_modules/tar/package.json:    "graceful-fs": "^3.0.2",
deps/npm/node_modules/node-gyp/package.json:    "graceful-fs": "3",
deps/npm/node_modules/sha/package.json:    "graceful-fs": "2 || 3",
deps/npm/node_modules/sha/package.json:    "graceful-fs": "2 || 3",
deps/npm/node_modules/fs-write-stream-atomic/package.json:    "graceful-fs": "^3.0.2"
deps/npm/node_modules/read-installed/node_modules/readdir-scoped-modules/package.json:    "graceful-fs": "^3.0.4",
deps/npm/node_modules/read-installed/package.json:    "graceful-fs": "2 || 3"
deps/npm/node_modules/read-installed/package.json:    "graceful-fs": "2 || 3"
deps/npm/node_modules/read-installed/test/fixtures/package.json:    "graceful-fs": "~2"
deps/npm/node_modules/fstream/package.json:    "graceful-fs": "3",
deps/npm/node_modules/tar/package.json:    "graceful-fs": "^3.0.2",
deps/npm/node_modules/write-file-atomic/package.json:    "graceful-fs": "^3.0.2",
deps/npm/node_modules/read-package-json/package.json:    "graceful-fs": "2 || 3"
deps/npm/node_modules/read-package-json/package.json:    "graceful-fs": "2 || 3"
deps/npm/node_modules/cmd-shim/package.json:    "graceful-fs": ">3.0.1 <4.0.0-0",
deps/npm/node_modules/npm-registry-client/package.json:    "graceful-fs": "^3.0.0",

thefourtheye · 2015-09-06T15:34:17Z

@bnoordhuis I'll submit a patch to those packages now. But then even if they upgrade, we can land this only after npm picks up the upgraded dependencies, right?

bnoordhuis · 2015-09-06T16:31:43Z

I'll submit a patch to those packages now. But then even if they upgrade, we can land this only after npm picks up the upgraded dependencies, right?

Yes, that's right.

othiym23 · 2015-09-06T17:24:51Z

Just so everyone is clear, even were we to land all those PRs today (which we're not going to do, because this is the middle of a long holiday weekend in the US, and also those changes need to go through our release process, and probably, y,know, be tested), it's at least two Thursdays until the patched version of npm would be ready for downstream use. This strongly argues against including this PR in Node 4.0.0, unless everyone is cool with waiting that long.

Big thanks to @thefourtheye for taking the time to put those PRs together.

thefourtheye · 2015-09-06T18:02:38Z

This strongly argues against including this PR in Node 4.0.0, unless everyone is cool with waiting that long.

As mentioned in the PR,

Do not merge yet; this is an experiment and for soliciting feedback.

we can assume that it may not be targeted for 4.0.0 (@bnoordhuis please correct if I am wrong).

Big thanks to @thefourtheye for taking the time to put those PRs together.

You are welcome 😊

bnoordhuis · 2015-09-06T18:05:52Z

we can assume that it may not be targeted for 4.0.0 (@bnoordhuis please correct if I am wrong)

Nope, that's right; I wasn't planning on landing this in v4.x.

chrisdickinson · 2015-09-08T17:16:12Z

lib/_stream_readable.js

+const debug = require('internal/debuglog')('stream');
+const inherits = require('internal/inherits');
+
+var Duplex;  // Break recursive dependency with _stream_duplex.js.


This seems like it should be unnecessary, since _stream_duplex's export is hoisted.

Fishrock123 · 2015-09-08T17:36:56Z

@Trott You know you can make a new label if you need to right? :)

graceful-fs used to monkey-patch node's core fs module. This has been fixed in the version 4. Related: nodejs/node#2714 PR-URL: #3

graceful-fs used to monkey-patch node's core fs module. This has been fixed in the version 4. Related: nodejs/node#2714 PR-URL: #4

graceful-fs used to monkey-patch node's core fs module. This has been fixed in the version 4. Related: nodejs/node#2714 PR-URL: #64

othiym23 · 2015-09-11T00:55:41Z

In [email protected], npm has landed all of the PRs that @thefourtheye made with graceful-fs updates for packages over which we have control. The only remaining holdouts are sha and cmd-shim, which @ForbesLindesay maintains, and in the case of sha, I'm trying to work out an issue related to npm's need to continue to support Node 0.8. In a release or two of npm, it should be clean enough to support this patch landing.

Fishrock123 · 2015-10-10T17:57:10Z

@bnoordhuis npm now ships with all those patches by default, so the above concerns seem to be met now.

Fishrock123 · 2016-01-13T16:25:47Z

@bnoordhuis is this still a thing you'd like to do?

bnoordhuis · 2016-02-12T21:46:48Z

Closing, stale. Now that it's possible to add our own code to the snapshot, that's probably a better solution anyway.

bnoordhuis mentioned this pull request Sep 6, 2015

Process startup time on ARM #2138

Closed

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55

silverwind mentioned this pull request Sep 6, 2015

A weird commit from 1970 #2713

Closed

bnoordhuis force-pushed the reduce-bootstrap-code branch from 517062d to 42e9c45 Compare September 6, 2015 14:38

thefourtheye reviewed Sep 6, 2015
View reviewed changes

thefourtheye mentioned this pull request Sep 6, 2015

dep: Update graceful-fs dependency to the latest nodejs/node-gyp#714

Closed

thefourtheye mentioned this pull request Sep 6, 2015

deps: Upgrade graceful-fs dependency to the latest npm/npm-registry-client#117

Closed

chrisdickinson reviewed Sep 8, 2015
View reviewed changes

zkat pushed a commit to npm/fs-vacuum that referenced this pull request Sep 9, 2015

deps: Upgrade graceful-fs dependency to the latest

874eb65

graceful-fs used to monkey-patch node's core fs module. This has been fixed in the version 4. Related: nodejs/node#2714 PR-URL: #3

zkat pushed a commit to npm/fs-write-stream-atomic that referenced this pull request Sep 9, 2015

deps: Upgrade graceful-fs dependency to the latest

032a98d

graceful-fs used to monkey-patch node's core fs module. This has been fixed in the version 4. Related: nodejs/node#2714 PR-URL: #4

zkat pushed a commit to isaacs/node-tar that referenced this pull request Sep 10, 2015

deps: Upgrade graceful-fs dependency to the latest

a6f60ef

graceful-fs used to monkey-patch node's core fs module. This has been fixed in the version 4. Related: nodejs/node#2714 PR-URL: #64

This was referenced Sep 11, 2015

node: deprecate public access to process.binding #2768

Closed

Uncaught TypeError: Object [object Object] has no method 'on' npm/fstream#46

Closed

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

bnoordhuis closed this Feb 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib,src: split up big builtin scripts #2714

lib,src: split up big builtin scripts #2714

bnoordhuis commented Sep 6, 2015

cjihrig commented Sep 6, 2015

ChALkeR commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

thefourtheye Sep 6, 2015

bnoordhuis commented Sep 6, 2015

thefourtheye commented Sep 6, 2015

Fishrock123 commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

thefourtheye commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

othiym23 commented Sep 6, 2015

thefourtheye commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

chrisdickinson Sep 8, 2015

Fishrock123 commented Sep 8, 2015

othiym23 commented Sep 11, 2015

Fishrock123 commented Oct 10, 2015

Fishrock123 commented Jan 13, 2016

bnoordhuis commented Feb 12, 2016

lib,src: split up big builtin scripts #2714

lib,src: split up big builtin scripts #2714

Conversation

bnoordhuis commented Sep 6, 2015

cjihrig commented Sep 6, 2015

ChALkeR commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

thefourtheye Sep 6, 2015

Choose a reason for hiding this comment

bnoordhuis commented Sep 6, 2015

thefourtheye commented Sep 6, 2015

Fishrock123 commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

thefourtheye commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

othiym23 commented Sep 6, 2015

thefourtheye commented Sep 6, 2015

bnoordhuis commented Sep 6, 2015

chrisdickinson Sep 8, 2015

Choose a reason for hiding this comment

Fishrock123 commented Sep 8, 2015

othiym23 commented Sep 11, 2015

Fishrock123 commented Oct 10, 2015

Fishrock123 commented Jan 13, 2016

bnoordhuis commented Feb 12, 2016