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.

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

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

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

bnoordhuis
Copy link
Member

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 bnoordhuis added the build Issues and PRs related to build files or the CI. label Feb 26, 2016
@bnoordhuis
Copy link
Member Author

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

/* Default calls `delete this`. */ \
} \
} id##_external_data;
NODE_NATIVES_MAP(V)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this under-indented?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@thefourtheye 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 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
Copy link
Member

jasnell commented Mar 2, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

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

@bnoordhuis
Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Mar 3, 2016

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

@bnoordhuis
Copy link
Member Author

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

@bnoordhuis
Copy link
Member Author

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

@bnoordhuis
Copy link
Member Author

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

@bnoordhuis
Copy link
Member Author

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

@bnoordhuis
Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Mar 7, 2016

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

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@bnoordhuis ... ping

@bnoordhuis
Copy link
Member Author

@bnoordhuis
Copy link
Member Author

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

Can I get a LGTM?

@eljefedelrodeodeljefe
Copy link
Contributor

LGTM

@eljefedelrodeodeljefe
Copy link
Contributor

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
Copy link
Contributor

@bnoordhuis It looks like win2008r2 VS2013 totally failed?

@bnoordhuis
Copy link
Member Author

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

@bnoordhuis bnoordhuis removed the blocked PRs that are blocked by other issues or PRs. label Oct 25, 2016
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>
@bnoordhuis bnoordhuis closed this Oct 25, 2016
@bnoordhuis bnoordhuis deleted the speed-up-start-up branch October 25, 2016 11:16
@bnoordhuis bnoordhuis merged commit e4290de into nodejs:master Oct 25, 2016
@evanlucas
Copy link
Contributor

Can this land on v7?

@bnoordhuis
Copy link
Member Author

Yes, it's semver-patch.

evanlucas pushed a commit that referenced this pull request Nov 3, 2016
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>
pmq20 added a commit to pmq20/node-packer that referenced this pull request Nov 11, 2016
* Those from v7 are causing memory insufficiency when compiling
* cf. nodejs/node#5458
@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
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>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. memory Issues and PRs related to the memory management or memory footprint. performance Issues and PRs related to the performance of Node.js. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants