FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: js2c-cache by indutny · Pull Request #4777 · nodejs/node · GitHub

/ node Public

tools: js2c-cache - #4777

Closed
indutny wants to merge 4 commits into
nodejs:masterfrom
indutny:feature/code-cache
ConversationCommits (4)ChecksFiles changed
Closed

tools: js2c-cache#4777
indutny wants to merge 4 commits into
nodejs:masterfrom
indutny:feature/code-cache

Conversation

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member

Use V8 Code caching when host_arch == target_arch. This commit may
slightly improve startup time, and definitely improves require times
(when doing many of them).

For example, executing following file:

var fs = require('fs');
var http = require('http');
var https = require('https');
var path = require('path');
var util = require('util');

Takes: ~74ms on master
Takes: ~54ms with this commit

Number were taken on OS X with 64-bit node.js binaries.

cc @nodejs/collaborators @nodejs/v8

rvagg reviewed Jan 20, 2016
View reviewed changes
Comment thread src/tls_wrap.cc Outdated

rvagg Jan 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what's going on here? accidental debug or something else, same with the return 0 below.

indutny Jan 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, accidental debug.

rvagg commented Jan 20, 2016

Copy link
Copy Markdown
Member

would we consider shipping the binary for this, with a more node-specific name than js2c-cache?

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

We don't need to ship this anywhere, it is run during build time.

alexlamsl reviewed Jan 20, 2016
View reviewed changes
Comment thread src/node.js Outdated

alexlamsl Jan 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looking at the definition of NativeModule.wrap 12 lines up - how is cached_data being used?

indutny Jan 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It isn't, I'm in the process of making it work.

alexlamsl Jan 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@indutny oh oops - sorry for the noise!

mscdex added tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Jan 20, 2016

rvagg commented Jan 20, 2016

Copy link
Copy Markdown
Member

@indutny so you figure that if anyone wants to make use of this kind of functionality outside of lib/*.js, like NW.js, they'll be using source and hacking Node a bit anyway?

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

@rvagg I haven't thought about it that far yet, and so far I'm not sure if it provides any significant speed boost after all. Need to finish it up to do benchmarks.

indutny force-pushed the feature/code-cache branch from 4a34cea to 37d76fa Compare January 20, 2016 09:36
indutny changed the title [WIP] tools: js2c-cache tools: js2c-cache Jan 20, 2016

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

Alright, I think the PR is ready now. Let's see what CI (especially ARM bots) think about it: https://ci.nodejs.org/job/node-test-pull-request/1314/

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

@rvagg regarding NW.js, there is a cached_data option for vm.ContextifyContext, so I guess they could try using it and obtaining this data through the js2c-cache.

bnoordhuis commented Jan 20, 2016

Copy link
Copy Markdown
Member

Data point, when I experimented with the preparser API a while ago (what corresponds to kProduceParserCache now) I saw no change in start-up times whatsoever.

Vyacheslav later told me it only makes a difference when the source code is 100s or 1000s of kilobytes big. Ours is in the low 100s but it's not loaded all at once.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

@bnoordhuis it seems that my small require benchmark disagrees with you...

thefourtheye reviewed Jan 20, 2016
View reviewed changes
Comment thread src/node.js Outdated

thefourtheye Jan 20, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Style nit: we prefer camel case for JavaScript variables no?

indutny Jan 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ack.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

@bnoordhuis also, it is not exactly preparser thing. It is caching compiled code, not parser data.

bnoordhuis commented Jan 20, 2016

Copy link
Copy Markdown
Member

Yes, I get that. I'm curious to see if it makes a real difference. Full-codegen is pretty quick but maybe not on ARM.

thefourtheye reviewed Jan 20, 2016
View reviewed changes
Comment thread tools/js2c-cache.cc Outdated

thefourtheye Jan 20, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Everywhere else nullptr is used. Is it okay to use NULL here?

indutny Jan 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ack.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

Looks like this js2c-cache does not compile on windows. Going to skip it for now, and iterate towards it sometime later (if I'll have any incentive).

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/1315/ (Previous is mostly green, except windows).

thefourtheye reviewed Jan 20, 2016
View reviewed changes
Comment thread tools/js2c.py Outdated

thefourtheye Jan 20, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unused function?

indutny Jan 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ack.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

Gosh, one more windows failure. Should be fixed now.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

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

benjamingr commented Jan 20, 2016

Copy link
Copy Markdown
Member

Sorry for the ignorance, but can this impact the speed of userlandrequire`s? Those can be a big problem in larger projects and if it's possible giving users the ability to cache if NODE_ENV is production would be a huge help.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

Pushed fix, new CI: https://ci.nodejs.org/job/node-test-pull-request/1323/

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

@benjamingr possibly no, fs overhead is very likely to be bigger than compilation overhead, but this PR opens new APIs for using code cache in ChildProcess.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

New CI with windows support: https://ci.nodejs.org/job/node-test-pull-request/1327/ (hopefully)

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

One more fix, and it should be good to go: https://ci.nodejs.org/job/node-test-pull-request/1329/

jbergstroem reviewed Jan 20, 2016
View reviewed changes
Comment thread tools/js2c.py Outdated

jbergstroem Jan 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perhaps use subprocess.Communicate to simplify? (written without testing)

output, error = p.communicate(input=line)
return output

indutny Jan 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Appears to be working locally, thank you for suggestion.

indutny commented Jan 20, 2016

Copy link
Copy Markdown
Member Author

Last CI was finally green. Please let me know if it LGTY.

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
vm: introduce cachedData/produceCachedData
67c6072 
Introduce `cachedData`/`produceCachedData` options for `v8.Script`.
Could be used to consume/produce V8's code cache for speeding up
compilation of known code.

PR-URL: nodejs#4777
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
indutny mentioned this pull request Jul 5, 2016
Merged

lin7sh commented Aug 27, 2017

Copy link
Copy Markdown

@hashseed Sound crazy, Do you mean use v8 as the vm platform for any language that can compile to this "reverse engineered byte code"? A bit of the overlapping to wasm, but I believe js bytecode has more feature than low level wasm

baby636 mentioned this pull request Oct 18, 2022
Open
kissofire mentioned this pull request Oct 18, 2022
Open
This was referenced Oct 18, 2022
Open
Open
Qdigital mentioned this pull request Oct 18, 2022
Open
This was referenced Oct 18, 2022
Open
Open
MaxMood96 mentioned this pull request Oct 18, 2022
Open
adamlaska mentioned this pull request Oct 18, 2022
Open
ryan-ally mentioned this pull request Oct 18, 2022
Open
adamlaska mentioned this pull request Oct 18, 2022
Open
snyk-bot mentioned this pull request Oct 18, 2022
Open
This was referenced Oct 18, 2022
Open
Open
snyk-bot mentioned this pull request Oct 18, 2022
Open
Verdinjoshua26 mentioned this pull request Oct 19, 2022
Open
This was referenced Oct 19, 2022
Open
Open
This was referenced Oct 19, 2022
Open
Open
baby636 mentioned this pull request Oct 19, 2022
Open
bsmoove247 mentioned this pull request Oct 19, 2022
Open
baby636 mentioned this pull request Oct 19, 2022
Open
ryan-ally mentioned this pull request Oct 19, 2022
Open
ron-a20 mentioned this pull request Oct 19, 2022
Open
jhudaz mentioned this pull request Oct 19, 2022
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

semver-minor PRs that contain new features and should be released in the next minor version. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL