phpv8 / v8js

V8 Javascript Engine for PHP — This PHP extension embeds the Google V8 Javascript Engine
http://pecl.php.net/package/v8js
MIT License
1.84k stars 200 forks source link

Possible to exclude default V8JS methods (sleep, exit, print, var_dump, require) #285

Closed virgofx closed 7 years ago

virgofx commented 7 years ago

Is it possible to exclude the default V8JS injected methods in the isolate?

1) Would this improve performance (even, if slightly, by way of reduced JS footprint and php hooks)? (Flag to remove https://github.com/phpv8/v8js/blob/php7/v8js_class.cc#L420) In high performance app if one can save a few milliseconds on the construction of the V8 instance -- this is huge.

2) Would allow for native handling of specific functions. E.g. require for CommonJS modules to allow default browser-packed (browserify, webpack) implementation.

stesie commented 7 years ago

Generally it is of course possible, currently there's only a work-around solution: run some JS code that deletes the methods from the global object, ... yet this of course doesn't improve the performance, it just makes them inaccessible to the code.

What would be your preferred way to disable method injection? I personally would go for a php.ini setting; I don't think it's helpful to have arguments (or flags) on V8Js constructor

Would you prefer seperate flags for each method that's being injected? So you can e.g. disable sleep or require, but leave the others untouched?

virgofx commented 7 years ago

Yeah, constructor args definitely can get polluted fast. INI flag works too although is project using that anywhere else? (Flags per method works as well - whatever is easier).

I'm curious if we can run benchmark (before/after) on removal of the default methods in V8 object. I know it's small but I'm guessing it's 1-2ms.

Deleting via JS globals not preferred approach because I'm trying to improve the construction time of the V8JS object. And it just seems more work-around'ish to have to remove after the fact. Would rather just not include them from the beginning.

stesie commented 7 years ago

Feel free to comment out that line and run some tests with and without it. But I'm pretty sure the difference is negligible (i.e. way <1ms).

Hence I'm yet hesitant to do something like that.

Generally there already are a few INI flags, yet it's not a very clean solution either.

stesie commented 7 years ago

hmm, ok, so you made me curious now ...

that's the script I've been testing with:

<?php

echo "first instantiation ...\n";
$start = microtime(true);
$v8 = new V8Js();
var_dump(microtime(true) - $start);

echo "testing mass instantiation ...\n";
$total = 0;
for($i = 0; $i < 1000; $i ++) {
    $start = microtime(true);
    $v8 = new V8Js();
    $total += (microtime(true) - $start);
}
var_dump($total / $i);

... after all it just creates 1000 V8Js instances (and destructs them immediately) and tells the average creation time.

With unmodified code from master branch every single object creation takes 0.00433 ms on my box (yet this value isn't exactly stable).

Commenting out the aforementioned line, object creation takes 0.00427 ms on average. Which is just a ~60 us difference ...

virgofx commented 7 years ago

small ... well performance not an issue. Just point 2 relevant it appears from initial post. Thanks for the quick benchmarks!

stesie commented 7 years ago

well, you can just redefine the require symbol on JS side (as you would anyhow), so if you don't mind that V8Js' version of require is available initially, then you can just override it and go with your own version

virgofx commented 7 years ago

yeah good point. since the performance impact is us and not ms will handle that case specifically. Thanks @stesie