Skip to content

Conversation

@indutny
Copy link
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

src/tls_wrap.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, accidental debug.

@rvagg
Copy link
Member

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

@indutny
Copy link
MemberAuthor

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

src/node.js Outdated

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@indutny oh oops - sorry for the noise!

@mscdexmscdex 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
Copy link
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
Copy link
MemberAuthor

@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.

@indutnyindutny changed the title [WIP] tools: js2c-cachetools: js2c-cacheJan 20, 2016
@indutny
Copy link
MemberAuthor

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

@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
Copy link
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
Copy link
MemberAuthor

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

src/node.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
MemberAuthor

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

@bnoordhuis
Copy link
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
MemberAuthor

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

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

tools/js2c.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused function?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
MemberAuthor

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

@indutny
Copy link
MemberAuthor

@benjamingr
Copy link
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
Copy link
MemberAuthor

@indutny
Copy link
MemberAuthor

@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
Copy link
MemberAuthor

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

@indutny
Copy link
MemberAuthor

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

tools/js2c.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Appears to be working locally, thank you for suggestion.

@indutny
Copy link
MemberAuthor

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
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 <[email protected]>
@lin7sh
Copy link

@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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@indutny@rvagg@bnoordhuis@benjamingr@hashseed@lin7sh@ofrobots@jbergstroem@trevnorris@clarle@thefourtheye@alexlamsl@mscdex