Skip to content

use vm.Script to avoid reparse #4892

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

Merged
merged 1 commit into from
Feb 13, 2017
Merged

Conversation

HerringtonDarkholme
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme commented Feb 9, 2017

DISCLAIMER: I don't have enough knowledge in nodejs nor V8. I read most cpp/internal js is by guessing and cramming on cppreference.com :/ I welcome any correction!

This pr tries to alleviate the problem in #4872. When users include third party library into server bundle, the parsing overhead can be smaller.

If user provided global variable is compared with another global variable in library code, they will never be equal because they are executed in different context.

To fix this generally, we have to run library code and user code in the same context. This means we need to parse a lot of code in one request.

We can skip parsing by creating a Script object, and runInNewContext is effectively a shortcut. Thus it is safe to reuse the Script object.

Reusing Script should save several C++ call and reduce garbage collection pressure since the underlying C++ object has a Persistent which will not be collected until out of memory (it is cargo cult to me, though).

However, I cannot see a performance improvement in a simple benchmark, regardless of whether bundling library code into server entry. (I use vue-hackernews as example, tested under ab -n 1000 -c 100).
It seems v8 will cache compiled code. So parsing overhead is relatively ignorable in real world.

But reusing object is still a good practice, so I submitted this pull request for symbolic value.

@yyx990803
Copy link
Member

Looks good to me! Can we add a test case for #4872?

@HerringtonDarkholme
Copy link
Member Author

Sadly #4872 is not fixed in this pull request 😞 . It only reduce reparsing overhead so more library code can be bundled at the same cost.

If we bundle Vue library into the server bundle, #4872 can be fixed. It has been verified by existing tests.
If Vue library is required from external, #4872 will still have problem due to different execution context.

We can reduce the reparsing overhead as is done here, so bundling Vue library is less expensive. We still need users to configure their project to bundle library code together with application code if they need to use APIs which compare global variables.

@yyx990803
Copy link
Member

Ok, I guess we can adjust prop type check strategy to not rely on globals instead.

@yyx990803 yyx990803 merged commit a0042c4 into vuejs:dev Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants