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.

Sign up for GitHub

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

Jump to bottom

tools: js2c-cache #4777

Closed
wants to merge 4 commits into from
Closed

tools: js2c-cache #4777

wants to merge 4 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 20, 2016

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
rvagg reviewed Jan 20, 2016
View reviewed changes
src/tls_wrap.cc
@@ -663,6 +663,7 @@ void TLSWrap::OnReadSelf(ssize_t nread,
Local<Object> buf_obj;
if (buf != nullptr)
buf_obj = Buffer::New(wrap->env(), buf->base, buf->len).ToLocalChecked();
fprintf(stderr, "%d\n", nread);
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
Member Author

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

rvagg commented Jan 20, 2016

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

@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

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

alexlamsl
alexlamsl reviewed Jan 20, 2016
View reviewed changes
src/node.js
@@ -984,7 +988,8 @@

NativeModule.prototype.compile = function() {
var source = NativeModule.getSource(this.id);
source = NativeModule.wrap(source);
var cached_data = NativeModule.getSourceCachedData(this.id);
source = NativeModule.wrap(source, cached_data);
Copy link

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
Member Author

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.

Copy link

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!

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

rvagg commented Jan 20, 2016

@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
Member Author

indutny commented Jan 20, 2016

@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 indutny force-pushed the feature/code-cache branch from 4a34cea to 37d76fa Compare January 20, 2016 09:36
@indutny indutny changed the title [WIP] tools: js2c-cache tools: js2c-cache Jan 20, 2016
@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

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
Member Author

indutny commented Jan 20, 2016

@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
Member Author

indutny commented Jan 20, 2016

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

thefourtheye
thefourtheye reviewed Jan 20, 2016
View reviewed changes
src/node.js
@@ -984,10 +988,12 @@

NativeModule.prototype.compile = function() {
var source = NativeModule.getSource(this.id);
var cached_data = NativeModule.getSourceCachedData(this.id);
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
Member Author

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

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

thefourtheye
thefourtheye reviewed Jan 20, 2016
View reviewed changes
tools/js2c-cache.cc
public:
virtual void* Allocate(size_t length) {
void* data = AllocateUninitialized(length);
return data == NULL ? data : memset(data, 0, length);
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
Member Author

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

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
Member Author

indutny commented Jan 20, 2016

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

thefourtheye
thefourtheye reviewed Jan 20, 2016
View reviewed changes
tools/js2c.py
@@ -387,10 +418,26 @@ def JS2C(source, target):
})
output.close()

def filter_js2c_cache(str):
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
Member Author

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

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

@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

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

@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
Member Author

indutny commented Jan 20, 2016

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

@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

@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
Member Author

indutny commented Jan 20, 2016

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

@indutny
Copy link
Member Author

indutny commented Jan 20, 2016

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

jbergstroem
jbergstroem reviewed Jan 20, 2016
View reviewed changes
tools/js2c.py
out += line

return out

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)
return output

Copy link
Member Author

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
Member Author

indutny commented Jan 20, 2016

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

@indutny indutny mentioned this pull request Jul 5, 2016
Merged
@lin7sh
Copy link

lin7sh commented Aug 27, 2017

@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 baby636 mentioned this pull request Oct 18, 2022
Open
@kissofire kissofire mentioned this pull request Oct 18, 2022
Open
This was referenced Oct 18, 2022
Open
Open
@Qdigital Qdigital mentioned this pull request Oct 18, 2022
Open
This was referenced Oct 18, 2022
Open
Open
@MaxMood96 MaxMood96 mentioned this pull request Oct 18, 2022
Open
@adamlaska adamlaska mentioned this pull request Oct 18, 2022
Open
@ryan-ally ryan-ally mentioned this pull request Oct 18, 2022
Open
@adamlaska adamlaska mentioned this pull request Oct 18, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Oct 18, 2022
Open
This was referenced Oct 18, 2022
Open
Open
@snyk-bot snyk-bot mentioned this pull request Oct 18, 2022
Open
@Verdinjoshua26 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 baby636 mentioned this pull request Oct 19, 2022
Open
@bsmoove247 bsmoove247 mentioned this pull request Oct 19, 2022
Open
@baby636 baby636 mentioned this pull request Oct 19, 2022
Open
@ryan-ally ryan-ally mentioned this pull request Oct 19, 2022
Open
@ron-a20 ron-a20 mentioned this pull request Oct 19, 2022
Open
@jhudaz jhudaz mentioned this pull request Oct 19, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Oct 21, 2022
Open
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.