src,tools: speed up startup by 2.5% by bnoordhuis · Pull Request #5458 · nodejs/node · GitHub

Use zero-copy external string resources for storing the built-in JS
source code. Saves a few hundred kilobyte of memory and consistently
speeds up benchmark/misc/startup.js by 2.5%.

Everything old is new again! Commit 74954ce ("Add string class that
uses ExternalAsciiStringResource.") from 2011 did the same thing but
I removed that in 2013 in commit 34b0a36 ("src: don't use NewExternal()
with unaligned strings") because of a limitation in the V8 API.

V8 no longer requires that strings are aligned if they are one-byte
strings so it should be safe to re-enable external strings again.

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

I didn't test but I expect that the speed-up on slower systems will be even more significant.

Isn't this under-indented?

We don't really seem to have a convention for indentation in macros but two spaces appears to be most common:

$ perl -ne 'if (/define V\(/ && /\\$/) { <> =~ /^(\s+)/; print "| ", length($1), " spaces\n" }' src/*.{cc,h} | sort | uniq -c | sort -nr
     16 | 2 spaces
      7 | 4 spaces
      1 | 6 spaces
      1 | 30 spaces

LGTM

Marking as don't land on v4 for now... can potentially revisit that if the v8 limitation is lifted there also

Crikey, looks like VS 2013 expands the macro wrong... 2015 gets it right but that's not much comfort.

O.o ... that's fun. VS 2013 is always full of wonderful surprises.

CI with workaround: https://ci.nodejs.org/job/node-test-pull-request/1860/

And another workaround... https://ci.nodejs.org/job/node-test-pull-request/1862/

One more VS fix-up... https://ci.nodejs.org/job/node-test-pull-request/1865/

Make that https://ci.nodejs.org/job/node-test-pull-request/1867/, Jenkins crapped out.

And again because the Windows status page kept timing out... https://ci.nodejs.org/job/node-test-pull-request/1870/

Appears to be a significant bit of breakage on one of the windows machines.

@bnoordhuis ... ping

Updated, PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/2641/

Rebased: https://ci.nodejs.org/job/node-test-pull-request/3120/

Can I get a LGTM?

LGTM

For the record: "Everything old is new again!" Is my absolute favourite commit of all time.

Is there a place where making VS2015 requirement for compilation was discussed? Probably not so easy, right? But definitely much more comfortable.

@bnoordhuis It looks like win2008r2 VS2013 totally failed?

Dusted off and rebased. CI: https://ci.nodejs.org/job/node-test-pull-request/4654/

Use zero-copy external string resources for storing the built-in JS source code. Saves a few hundred kilobyte of memory and consistently speeds up `benchmark/misc/startup.js` by 2.5%. Everything old is new again! Commit 74954ce ("Add string class that uses ExternalAsciiStringResource.") from 2011 did the same thing but I removed that in 2013 in commit 34b0a36 ("src: don't use NewExternal() with unaligned strings") because of a limitation in the V8 API. V8 no longer requires that strings are aligned if they are one-byte strings so it should be safe to re-enable external strings again. PR-URL: nodejs#5458 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

Can this land on v7?

Yes, it's semver-patch.

Use zero-copy external string resources for storing the built-in JS source code. Saves a few hundred kilobyte of memory and consistently speeds up `benchmark/misc/startup.js` by 2.5%. Everything old is new again! Commit 74954ce ("Add string class that uses ExternalAsciiStringResource.") from 2011 did the same thing but I removed that in 2013 in commit 34b0a36 ("src: don't use NewExternal() with unaligned strings") because of a limitation in the V8 API. V8 no longer requires that strings are aligned if they are one-byte strings so it should be safe to re-enable external strings again. PR-URL: #5458 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

* Those from v7 are causing memory insufficiency when compiling * cf. nodejs/node#5458

I've gone ahead and backported this to v6.x. I believe it has spent enough time maturing on v7.x

Please feel free to let me know if you think it needs to spend more time before backport

Use zero-copy external string resources for storing the built-in JS source code. Saves a few hundred kilobyte of memory and consistently speeds up `benchmark/misc/startup.js` by 2.5%. Everything old is new again! Commit 74954ce ("Add string class that uses ExternalAsciiStringResource.") from 2011 did the same thing but I removed that in 2013 in commit 34b0a36 ("src: don't use NewExternal() with unaligned strings") because of a limitation in the V8 API. V8 no longer requires that strings are aligned if they are one-byte strings so it should be safe to re-enable external strings again. PR-URL: #5458 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

Use zero-copy external string resources for storing the built-in JS source code. Saves a few hundred kilobyte of memory and consistently speeds up `benchmark/misc/startup.js` by 2.5%. Everything old is new again! Commit 74954ce ("Add string class that uses ExternalAsciiStringResource.") from 2011 did the same thing but I removed that in 2013 in commit 34b0a36 ("src: don't use NewExternal() with unaligned strings") because of a limitation in the V8 API. V8 no longer requires that strings are aligned if they are one-byte strings so it should be safe to re-enable external strings again. PR-URL: nodejs/node#5458 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

bnoordhuis added the build Issues and PRs related to build files or the CI. label Feb 26, 2016

thefourtheye reviewed Feb 26, 2016
View reviewed changes

thefourtheye added tools Issues and PRs related to the tools directory. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 26, 2016

ChALkeR added performance Issues and PRs related to the performance of Node.js. memory Issues and PRs related to the memory management or memory footprint. labels Feb 27, 2016

jasnell added the dont-land-on-v4.x label Mar 2, 2016

bnoordhuis force-pushed the speed-up-start-up branch from 79f85ba to 90aa74b Compare March 5, 2016 12:45

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

bnoordhuis force-pushed the speed-up-start-up branch from 563fad5 to 364e887 Compare May 13, 2016 21:25

bnoordhuis force-pushed the speed-up-start-up branch from 364e887 to 85481f7 Compare June 29, 2016 11:37

bnoordhuis mentioned this pull request Jun 29, 2016

build: reduces unused js2c.py code #7465

Closed

4 tasks

bnoordhuis mentioned this pull request Aug 6, 2016

build: drop support for VS 2013 in v6 LTS #7989

Closed

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

bnoordhuis force-pushed the speed-up-start-up branch from 69c38fe to abaa566 Compare October 24, 2016 21:29

bnoordhuis removed the blocked PRs that are blocked by other issues or PRs. label Oct 25, 2016

bnoordhuis force-pushed the speed-up-start-up branch from abaa566 to e4290de Compare October 25, 2016 11:16

bnoordhuis closed this Oct 25, 2016

bnoordhuis deleted the speed-up-start-up branch October 25, 2016 11:16

bnoordhuis merged commit e4290de into nodejs:master Oct 25, 2016

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

pmq20 added a commit to pmq20/node-packer that referenced this pull request Nov 11, 2016

Use node_javascript.cc and js2c.py from v6

48428dd

* Those from v7 are causing memory insufficiency when compiling * cf. nodejs/node#5458

MylesBorins added the lts-watch-v6.x label Nov 18, 2016

Qard mentioned this pull request Nov 19, 2016

backport: src,tools: speed up startup by 2.5% #9693

Closed

2 tasks

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Jan 23, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

This was referenced Feb 2, 2017

utf8 problem in _third_party_main.js #10673

Closed

src: support UTF-8 in compiled-in JS source files #11129

Merged

aqrln mentioned this pull request Feb 14, 2017

tools: add ASCII only lint rule in lib/ #11371

Closed

3 tasks

src,tools: speed up startup by 2.5% by bnoordhuis · Pull Request #5458 · nodejs/node · GitHub

Uh oh!

Uh oh!

src,tools: speed up startup by 2.5% - #5458

src,tools: speed up startup by 2.5%#5458bnoordhuis merged 1 commit intonodejs:masternodejs/node:masterfrom bnoordhuis:speed-up-start-upbnoordhuis/io.js:speed-up-start-upCopy head branch name to clipboard

Conversation

bnoordhuis commented Feb 26, 2016

Uh oh!

bnoordhuis commented Feb 26, 2016

Uh oh!

thefourtheye Feb 26, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 27, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

bnoordhuis commented Mar 2, 2016

Uh oh!

jasnell commented Mar 3, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 6, 2016

Uh oh!

jasnell commented Mar 7, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

bnoordhuis commented May 13, 2016

Uh oh!

bnoordhuis commented Jun 29, 2016

Uh oh!

eljefedelrodeodeljefe commented Jun 29, 2016

Uh oh!

eljefedelrodeodeljefe commented Jun 29, 2016

Uh oh!

Fishrock123 commented Jun 29, 2016

Uh oh!

bnoordhuis commented Oct 24, 2016

Uh oh!

evanlucas commented Nov 2, 2016

Uh oh!

bnoordhuis commented Nov 3, 2016

Uh oh!

MylesBorins commented Jan 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src,tools: speed up startup by 2.5%#5458
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:speed-up-start-up