Feature suggestion: compile scripts

[rosmo]

V8Js::executeString() now compiles and runs scripts each time it's called. It might be worth to add a V8Js::compileString() method which returns a resource for the compiled script and a V8Js::runScript() function to execute them.

[satoshi75nakamoto]

@rosmo — Yeah that might just make a bit more sense. I wonder what others think?

[stesie]

:+1:

... but I think we should leave executeString around. The recently added checkString method might be a good candidate for deprecation, in favour of compileString.

[rosmo]

Sounds good. I already wrote preliminary version of the changes needed, but I'll polish them a bit. I chose the resource way, since it's much less work.

[rosmo]

Here's the changes: https://github.com/rosmo/v8js/commits/compile-script

[stesie]

Hi @rosmo,

please see my inline comments to your commits.

Besides that

• I would prefer an implementation of executeString subsequently calling compileString and executeScript (+ free resource) to not duplicate so much code (both execute functions share about 120 lines of unmodified code)
• what about declaring checkString deprecated and using compileString already internally + freeing the resource immediately? (and if we don't deprecate the symbol, I still think that one can call the other + free resource to not duplicate the code)

cheers ~stesie

[rosmo]

Totally forgot about checkString(). And you're right about the duplication. I'll fix the issues ASAP.

[rosmo]

Here's a better branch: https://github.com/rosmo/v8js/commits/compile-script-fix

I'm not sure about the last commit though, because I'm not that familiar with V8. I thought the previous style was leaking memory, but it seems like the leaking is happening with the master branch as well (Apache's RSS seems to grow and not stabilize).

[rosmo]

The memory leak could be issue #71 related though.

[rosmo]

I updated the compile branch: https://github.com/rosmo/v8js/commits/compile-script-201409

[stesie]

@rosmo thanks for your contribution!

I just changed checkString (though deprecated) to use your new functions likewise.

cheers ~stesie

[stesie]

hmm, seems like this breaks the test fatal_error_recursive.php at least with certain v8 versions

3.29.1 regularly crashes with this trace

#0  0x00007ffff382c8de in v8::internal::IncrementalMarkingRootMarkingVisitor::VisitPointer(v8::internal::Object**) ()
   from /var/lib/jenkins/workspace/v8js-with-v8-3.29.1/v8-3.29.1/lib/libv8.so
#1  0x00007ffff38ea5cd in v8::internal::Isolate::Iterate(v8::internal::ObjectVisitor*, v8::internal::ThreadLocalTop*) ()
   from /var/lib/jenkins/workspace/v8js-with-v8-3.29.1/v8-3.29.1/lib/libv8.so
#2  0x00007ffff3810873 in v8::internal::Heap::IterateStrongRoots(v8::internal::ObjectVisitor*, v8::internal::VisitMode) ()
   from /var/lib/jenkins/workspace/v8js-with-v8-3.29.1/v8-3.29.1/lib/libv8.so
#3  0x00007ffff382d6ed in v8::internal::IncrementalMarking::StartMarking(v8::internal::IncrementalMarking::CompactionFlag) ()
   from /var/lib/jenkins/workspace/v8js-with-v8-3.29.1/v8-3.29.1/lib/libv8.so
#4  0x00007ffff382e2a3 in v8::internal::IncrementalMarking::Start(v8::internal::IncrementalMarking::CompactionFlag) ()
   from /var/lib/jenkins/workspace/v8js-with-v8-3.29.1/v8-3.29.1/lib/libv8.so
#5  0x00007ffff381f55e in v8::internal::Heap::IdleNotification(int) ()
   from /var/lib/jenkins/workspace/v8js-with-v8-3.29.1/v8-3.29.1/lib/libv8.so
#6  0x00007ffff3f46c2e in php_v8js_free_storage (object=0x7ffff7fd56a8) at /home/stesie/Projekte/v8js/v8js.cc:694
#7  0x00000000006c6d17 in zend_objects_store_free_object_storage ()
#8  0x00000000006912dc in ?? ()
#9  0x000000000069fd85 in ?? ()
#10 0x000000000063f47a in php_request_shutdown ()
#11 0x0000000000749fe4 in ?? ()
#12 0x000000000043141a in ?? ()
#13 0x00007ffff4d62b45 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6
#14 0x00000000004314ad in _start ()

[stesie]

hmmm, I'll close this again, since it doesn't seem to be the cause of the problem.

The problem probably is still only the stack unwinding stuff, the commit above makes it better but it's still not stable. Jenkins tells v8 versions 3.29.54 to 3.29.66 are unstable (i.e. the aforementioned test fails) and all other versions from 3.27.1 on are stable. However I cannot verify that on a second box.

And I just now noticed that subsequent requests (within the same php process) to one with a unwound fatal error just fail. I'll follow up with a specific bug report.

[rosmo]

Could we blacklist some versions if they are unstable?

[stesie]

@rosmo I don't think that blacklisting is the way to go. The unwinding still has its flaws (two less since today, but still at least one more) and leads to crashes here and there. I can reproduce crashes with versions 3.29.54 to 3.29.66 on two boxes with PHP 5.4; but not with 5.5 (which crashes with 3.25.30 though).

[rosmo]

@stesie There is a difference/bug when doing previous executeString and executeScript (we noticed this while doing an upgrade). I will try to produce a test case for this.