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

Memory leaks PHP5.5 apache mod_php #98

Closed camspiers closed 10 years ago

camspiers commented 10 years ago

I am experiencing memory leaks using v8js with PHP 5.5 and mod_php.

<?php
$v = new V8Js;

With a simple php file that creates a V8Js instance, I see memory used by apache increasing on every request. I also see the thread count increasing every request.

Making 100 requests to this simple script takes the memory usage from 1.1MB to 234.5MB. I see the thread count go from 1 to 506 threads.

satoshi75nakamoto commented 10 years ago

@camspiers — I'll try some tests at get back to you.

camspiers commented 10 years ago

Thanks!

camspiers commented 10 years ago

For some more information:

ProductName:    Mac OS X
ProductVersion: 10.8.4
BuildVersion:   12E55
PHP 5.5.13 (cli) (built: Jun 11 2014 11:27:56)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
v8js

V8 Javascript Engine => enabled
V8 Engine Compiled Version => 3.25.30
V8 Engine Linked Version => 3.25.30
Version => 0.1.5 // this is actually 1d8ae1f

Directive => Local Value => Master Value
v8js.flags => no value => no value
v8js.use_date => 0 => 0
[PHP Modules]
bcmath
bz2
calendar
Core
ctype
curl
date
dba
dom
ereg
exif
fileinfo
filter
ftp
gd
gettext
hash
iconv
intl
json
ldap
libevent
libxml
mbstring
memcache
mhash
mysql
mysqli
mysqlnd
odbc
openssl
pcntl
pcre
PDO
pdo_mysql
PDO_ODBC
pdo_sqlite
Phar
posix
readline
Reflection
session
shmop
SimpleXML
snmp
soap
sockets
SPL
sqlite3
standard
sysvmsg
sysvsem
sysvshm
tokenizer
v8js
wddx
xml
xmlreader
xmlrpc
xmlwriter
xsl
zip
zlib
rosmo commented 10 years ago

@camspiers Will the memory and thread count grow indefinitely or does it stabilize at some point?

camspiers commented 10 years ago

Hmm, I am not sure, I can look into it but there appears to be an issue regardless of the answer to that question. The memory gets high very quickly, like hundreds of MB. I predict that there will be problems as soon as you get the ulimit of apache.

camspiers commented 10 years ago

Using the php development server and the testing script used above. I can get the server to stop responding when it reaches 2048 threads. At this point it is consuming 933MB of RAM.

php -S localhost:8000
ab -n 1000 http://localhost:8000/testing.php
rosmo commented 10 years ago

I believe it's pretty normal for the virtual memory size (VSZ) to grow pretty large, but the RSS (resident set size) should not keep growing. I wonder if adding a v8::V8::ContextDisposedNotification before the IdleNotification would help...

rosmo commented 10 years ago

Also, could you try an older V8 version, just in case that has something to do with it? (maybe 3.22.3)

rosmo commented 10 years ago

All right, I tried with both V8 3.24.9 and 3.22.24.21 with this simple test script:

<?php
{
    $v8js = new \V8Js('PHP');
    $v8js->foo = 'Hello World';
    $v8js->print = function ($msg) { echo $msg; };
    $v8js->executeString('PHP.print(PHP.foo + "\n");');
    $v8js->foo = NULL;
    $v8js->print = NULL;
    $v8js = NULL;
}

Both versions of V8 leak memory rapidly (3.22.24.21 seems to leak faster). ContextDisposedNotification doesn't seem to have any effect. Actually even just doing new \V8Js and nothing else leaks memory.

rosmo commented 10 years ago

@camspiers Can you try this branch: https://github.com/rosmo/v8js/tree/dispose-isolate

camspiers commented 10 years ago

Hey @rosmo I will hopefully get a chance to do this over the next couple of days. Compiling on OS X isn't a quick task.

stesie commented 10 years ago

Hey @rosmo,

I can confirm that this patch indeed fixes the memory (and thread count) leakage. I've tested some v8 versions with PHP-internal server ...

however there's a regression with regard to the tests/function_passback.phpt test, which segfaults after applying your patch:

#0  0x00007ffff381d09e in v8::internal::GlobalHandles::Destroy(v8::internal::Object**) () from /var/lib/jenkins/workspace/v8js-with-v8-3.28.22/v8-3.28.22/lib/libv8.so
#1  0x00007ffff3f41ab9 in Reset (this=0x7ffff7fd36c8) at /var/lib/jenkins/workspace/v8js-with-v8-3.28.22/v8-3.28.22/include/v8.h:5843
#2  php_v8js_v8_free_storage (object=0x7ffff7fd36a8, handle=<optimized out>) at /home/stesie/Projekte/v8js/v8js.cc:500
#3  0x00000000006c6dd3 in zend_objects_store_del_ref_by_handle_ex ()
#4  0x00000000006c6df3 in zend_objects_store_del_ref ()
#5  0x000000000069090a in _zval_ptr_dtor ()
#6  0x00000000006adb38 in zend_hash_destroy ()
#7  0x000000000069e927 in _zval_dtor_func ()
#8  0x000000000069090a in _zval_ptr_dtor ()
#9  0x00000000006ac505 in ?? ()
#10 0x00000000006adcf8 in zend_hash_graceful_reverse_destroy ()
#11 0x000000000069106e in ?? ()
#12 0x000000000069f915 in ?? ()
#13 0x000000000063f0aa in php_request_shutdown ()
#14 0x0000000000749bb4 in ?? ()
#15 0x00000000004313ba in ?? ()
#16 0x00007ffff4d5fb45 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6
#17 0x000000000043144d in _start ()

... and that's actually why the Isolate disposal is still missing. I've actually noticed it some months ago.

The problem is that there can be references from PHP to V8 which remain until after the V8Js object (and in turn the Isolate) is disposed. If the PHP objects are freed later on, the weak references to V8 are reset and yield the crash from above.

Besides it might be problematic if the V8Js object is destroyed before the interpreter shuts down, e.g. if the context with the V8Js instance is left.

cheers ~stesie

stesie commented 10 years ago

Hey @rosmo,

I just had a closer look on the crash outlined above. The problem indeed is that the Reset on the v8::Persistent by the php-side weak object uses the isolate disposed earlier.

Since we cannot get rid of the weak objects there are two options

Since I'd assume the former list tends to get long and the latter to almost always be no more then one or two entries, I decided to go for the second option.

There might be a third approach by iterating all PHP objects and checking whether it's the same isolate ... and then immediately Reset that. I haven't checked whether an extension can do that.

Here's my extended version of your patch: https://github.com/stesie/v8js/tree/dispose-isolate

Please let me know what you think about it. I'd merge if you agree

cheers ~stesie

stesie commented 10 years ago

... ok, just switched to the other approach, i.e. keep a list of V8Object/V8Function instances created and update them, if the V8Js object is destroyed before those.

We don't only have to handle the cleanup situation well, but PHP code can always destroy the V8Js object and keep a V8Object around ... which would segfault if we don't handle it

With the approach of a list of disposed isolates we would have to iterate this list over and over again (on each and every access of such an object). With the list of V8Object instances there's just a little overhead on creation & destruction of those objects ...

Just pushed to the aforementioned branch

rosmo commented 10 years ago

@stesie, the patches seem to work ok (I merged master and your branch into https://github.com/rosmo/v8js/tree/compile-script-fix-isolate), so merging is ok by me.

camspiers commented 10 years ago

Thanks for all the effort on this ticket!