-
Notifications
You must be signed in to change notification settings - Fork 203
Handle V8 heap snapshots well + allow custom snapshot generation #207
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
Conversation
Looks like we have to test for internal functions unfortunately since the public V8 snapshot API is available no matter whether the library really supports it or not.
The test on internal symbols seems too fragile, e.g. with V8 version 4.5.90 it is false positive.
|
@stesie I'm getting test failures o.O
|
|
Please provide the contents of one out file, e.g. |
|
Rebuilding again from fresh V8-5.0.104 with this PR. Error during configure: ./configure Where can I provide the path to the native blob (Currently located in Update: I guess we need to copy the *.bin files into the |
|
@stesie I'm still getting this |
|
@virgofx As to from my PPA for libv8 (https://launchpad.net/~pinepain/+archive/ubuntu/libv8-4.10): # for some reason just snapshot=on for make doesn't work
# and v8 starts built with V8_USE_EXTERNAL_STARTUP_DATA which is not what we want.
# This is a simple workaround:
GYPFLAGS += -Dv8_use_snapshot=true -Dv8_use_external_startup_data=0
DEB_MAKE_EXTRA_ARGS += library=shared snapshot=on i18nsupport=onsame for homebrew recipe, if you are on OS X and use homebrew - ping me and I'll share recipe. If you are on ubuntu feels free to use my PPA, it but pay attention while I update it time to time and due to v8 API BC incompatible changes some update may be incompatible with v8js. It is less likely for 4.10 branch while I have in plan to start building 5.x soon. @stesie ping me so maybe we'll try to figure out about providing libv8 on ubuntu that will fits v8js versions, currently I build it for my own php-v8 extension. |
|
@virgofx yes,
further TODOs left
The Invariant Violation: Trying to release an instance into a pool of a different type exception is thrown by React. So actually you already have it kindof working. I haven't used React before so I'm unsure what really causes it. To get it working I initially just commented it out, then noticed further "real" problems and once those were sorted out I could savely comment it back in and thigs seem to work fine. Actually I had to fix two problems: I had |
|
|
||
|
|
||
| /* {{{ arginfo */ | ||
| ZEND_BEGIN_ARG_INFO_EX(arginfo_v8js_construct, 0, 0, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L1119 - L1124: I notice there is no argument defined for the new snapshot blob which is defined as a parameter above?
|
Thanks for the help guys. I have managed to get things worked externally but not able to use the Environment
Extensions loaded, PHP working ✔ Custom Snapshot DataManuallyCreate snapshot via Via PHPCan't get anything to work with the API?
|
|
@stesie Manually compiling, I can at least get back to the runtime Invariant issue associated with React + snapshots. What's weird is that it works using the same concatenated data via execute string; however, not as a snapshot. If you do have an exact binary of the included script you used for working React server side, let me know. I'm currently using: |
|
@stesie Removing the Math.random()'s did fix the invariant problem, allowing React to render server side. I now have timing results =) Will post on main issue. I confirm similar results to you! |
|
@stesie Do you know if the Math.random() issue is V8 related or V8JS related? I originally thought it was React related; however, I can reproduce the problem in V8JS by attempting to use Math.random() outside the global scope. e.g. random1 = Math.random();
function getRandom() {
return Math.random();
}print(random1); // Works
print(getRandom()); // Bugs out |
|
@virgofx very cool that it finally works for you too! :-) Regarding the arginfo, that one is "just" for PHP's reflection stuff, hence yes,
... but it is not relevant to execution. Have you re-tried snapshot passing via BTW the stesie@hahnschaaf:~/Projekte/v8js$ php5 -n -d extension_dir=./modules -d extension=v8js.so -d extension=readline.so -a
Interactive mode enabled
php > $snapshotBlob = V8Js::createSnapshot(file_get_contents('doublify.js'));
php > $v8 = new V8Js('PHP', [], [], true, $snapshotBlob);
php > $v8->executeString('print(doublify(23));');
46Regarding ... and to show it's not related to V8Js, you can simply reproduce it with V8's own shell (and no V8Js code at all): Create a file test.js: function getRandom() {
return Math.random();
}
var randomValue = Math.random();
var randomValueFromFunction = getRandom();create a snapshot stesie@hahnschaaf:~/Projekte/v8/out/native$ ./mksnapshot --startup_blob=snapshot_blob.bin test.js
Embedding extra script: test.jsstart a shell that uses the startup_blob stesie@hahnschaaf:~/Projekte/v8/out/native$ ./shell
V8 version 5.1.53 [sample shell]
> print(randomValue);
0.47515791309267663
> print(randomValueFromFunction);
0.05808531226718627
> print(getRandom());
<embedded script>:6: TypeError: Cannot read property '4' of undefined
return Math.random();
^
TypeError: Cannot read property '4' of undefined
at Object.random (native)
at getRandom (<embedded script>:6:15)
at (shell):1:7 |
This does *not* seem to depend on whether V8 itself was compiled with support for snapshots or not. Therefore use PHP_V8_USE_EXTERNAL_STARTUP_DATA only to mark whether we need to provide external snapshot data to V8.
|
@stesie Just some more notes for |
|
Submitted an issue over at V8 issue tracker. For now anyone who is following this can semi-polyfill by creating snapshot with a function to handle runtime generation by simply overwriting the Math.random = function () {
// Quick, simple version.
return Date.now() / 3000000000000;
} |
|
another work-around: overwrite $this->v8 = new V8Js('PHP', [], [], true, $blob);
$this->v8->rand = function() { return mt_rand() / mt_getrandmax(); };
$this->v8->executeString('Math.random = PHP.rand; ');... works like a charm and eliminates the ReactJS error message as well :) |
|
That's awesome! Didn't know you could just assign and reference functions like that! I'm testing the snapshot via construct as well. I don't believe there will be any timing differences, but am going to check :) |
|
Snapshot via constructor now works, Timing equivalent. I'm integrating all these changes into my V8JS PHP wrapper that handles caching. Latest tests passed as well (except for one skipped). Looks good, maybe time to merge upstream and then make a note about the One last thing: What do you think about creating a separate repository just for the stubs (Currently listed in the readme?) Basically, this would allow developers to update composer and then have full autocomplete documentation within IDE. It could be within the same repository; however, since it's really just a development thing doesn't make sense to pull the V8JS source in a PHP application. The current Readme would then be updated to the adjacent repository, which would be tagged as same versions as this respository. |
|
@virgofx I'm not so much an IDE user, still using good ol' Emacs :-) Documentation actually is a good topic, since most of it + the samples are pretty outdated and not always "best practice" (in my sense) ... |
Handle V8 heap snapshots well + allow custom snapshot generation
v8::V8::InitializeExternalStartupDataas needed, if yesV8Js::createSnapshotwhich creates a new heap snapshot (blob returned to caller as a string)V8Js::__constructas new fifth argumentThis refs issue #205