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

bnoordhuis · 2016-02-26T21:01:17Z

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/

bnoordhuis · 2016-02-26T21:03:39Z

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

thefourtheye · 2016-02-26T22:04:18Z

src/node_javascript.cc

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

jasnell · 2016-03-02T17:39:21Z

LGTM

jasnell · 2016-03-02T17:39:58Z

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

bnoordhuis · 2016-03-02T22:33:49Z

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

jasnell · 2016-03-03T18:08:56Z

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

bnoordhuis · 2016-03-05T12:46:10Z

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

bnoordhuis · 2016-03-05T13:43:59Z

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

bnoordhuis · 2016-03-05T18:09:49Z

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

bnoordhuis · 2016-03-05T18:12:10Z

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

bnoordhuis · 2016-03-06T10:32:01Z

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

jasnell · 2016-03-07T18:34:13Z

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

jasnell · 2016-03-21T23:46:45Z

@bnoordhuis ... ping

bnoordhuis · 2016-05-13T21:27:00Z

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

bnoordhuis · 2016-06-29T11:39:57Z

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

Can I get a LGTM?

eljefedelrodeodeljefe · 2016-06-29T11:51:20Z

LGTM

eljefedelrodeodeljefe · 2016-06-29T11:57:33Z

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.

Fishrock123 · 2016-06-29T12:20:58Z

@bnoordhuis It looks like win2008r2 VS2013 totally failed?

bnoordhuis · 2016-10-24T21:30:35Z

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 <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

evanlucas · 2016-11-02T18:18:15Z

Can this land on v7?

bnoordhuis · 2016-11-03T04:19:32Z

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 <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

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

MylesBorins · 2017-01-23T01:30:51Z

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 <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

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 <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

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 90aa74bCompare March 5, 2016 12:45

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

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

bnoordhuis force-pushed the speed-up-start-up branch from 364e887 to 85481f7Compare 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 c5ce7f4Compare September 21, 2016 00:09

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

bnoordhuis force-pushed the speed-up-start-up branch from 69c38fe to abaa566Compare 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 e4290deCompare 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:masterOct 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

Uh oh!

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

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

Uh oh!

Conversation

bnoordhuis commented Feb 26, 2016

Uh oh!

bnoordhuis commented Feb 26, 2016

Uh oh!

thefourtheyeFeb 26, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuisFeb 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