jbboehr / php-mustache

Mustache PHP Extension
MIT License
56 stars 22 forks source link

Can't store MustacheTemplate in APC #2

Closed woledzki closed 11 years ago

woledzki commented 11 years ago

When you try to store MustacheTemplate in APC, you will get en error: "MustacheTemplate was not initialized properly"

I think it's because you try to get you class from PHP process store.

php_obj_MustacheTemplate * payload = 
            (php_obj_MustacheTemplate *) zend_object_store_get_object(_this_zval TSRMLS_CC);

Scenario:

Is there any way to cache compiled templates?

jbboehr commented 11 years ago

Yeah, the classes don't implement the serialize handlers. I'm not sure there would be a performance advantage as I'd have to serialize the C representation of the template and it's hard to say if unserializing it would be significantly faster than just re-parsing it.

woledzki commented 11 years ago

I'm not sure if it needs to implement serialize/de-serialize to be stored in APC.

I will try to play with it locally - but that will be my first steps in php extension, but hey, you have to start somewhere.

jbboehr commented 11 years ago

If the data was stored as a PHP object property, it would work fine, however I'm storing the string and the parsed template in C, so it's not touched by the serializer afaik. It would be easy to add support to just store the template string.

woledzki commented 11 years ago

any chance for adding mustache_node_from_zval and MustacheTemplate MustacheTemplate::createFromArray(array $tpl) or adding optional __construct parameter?

jbboehr commented 11 years ago

mustache_node_to_zval was mainly meant to aid debugging, not to store a compiled version of the template. I can add mustache_node_from_zval and createFromArray, but my guess is it won't be faster. In any case, once mustache_node_from_zval is written, I can implement the serialize handlers pretty easily as that function would do all of the work.

woledzki commented 11 years ago

Thanks! I will give it a go

jbboehr commented 11 years ago

No problem. The test has an example: https://github.com/jbboehr/php-mustache/blob/master/tests/MustacheTemplate____wakeup-apc.phpt

jbboehr commented 11 years ago

If I get a chance, I'll run some benchmarks on it vs reparsing.

woledzki commented 11 years ago

I've made a quick POK where I use your extension Mustache->tokenize() to generate and save classes with static method render. So far it's the fastest way to render templates.

5,000,000 loops
STATIC CLASS: TIME: 00:17s INCLUDE PHTM: TIME: 00:20s EXTENSION (COMPILED): TIME: 00:22s

echo "\n\n STATIC CLASS...\n";
$tic = time();
for ($i = 0; $i < $n; $i++) {
    $o = \TEST_TPL::render($data);
}
$toc = time();
echo "\n TIME: " . date('i:s', ($toc- $tic));

echo "\n\n INCLUDE PHTML...\n";
$tic = time();
for ($i = 0; $i < $n; $i++) {
    ob_start();
    include __DIR__ . '/tpl-test.phtml';
    ob_end_clean();
}
$toc = time();
echo "\n TIME: " . date('i:s', ($toc- $tic));

echo "\n\n EXTENSION (COMPILED)...\n";
$mustache = new \Mustache();
$tpl = $mustache->compile($tmpl);
$tic = time();
for ($i = 0; $i < $n; $i++) {
    $o = $mustache->render($tpl, $data);
}
$toc = time();
echo "\n TIME: " . date('i:s', ($toc- $tic));

PS. There are still some problems with memory leaks - your extension used 2GB for that run :(

jbboehr commented 11 years ago

Ah weird. I ran valgrind on it but I didn't see anything coming from my extension. I'll try compiling PHP in debug mode and see if that works better.

Is the static method using serialize or APC, or is it just storing the compiled template object in an array?

woledzki commented 11 years ago

Render function is just return "html {$variables} and function calls for sections"

I will send you a link with example file.

jbboehr commented 11 years ago

It's weird. I'm running Valgrind on the extension compile loop above, and while Valgrind doesn't detect any leaks coming from my extension, the memory usage is still really high. I'm going to try compiling PHP in debug mode and see if that helps.

jbboehr commented 11 years ago

Heh I think I got them. My calls to zend_hash_get_current_key_ex were duplicating the key string twice.


foreach( array(1, 100, 10000, 1000000) as $n ) {
  echo "N=", $n, "\n";
  $mustache = new \Mustache();
  $tpl = $mustache->compile('{{test}}');
  $data = array('test' => 'bar');
  $tic = time();
  for ($i = 0; $i < $n; $i++) {
      $o = $mustache->render($tpl, $data);
  }
  $toc = time();
  echo "TIME: " . date('i:s', ($toc- $tic)), "\n";
  echo "MEMREAL: ", memory_get_usage(true), "\n";
  echo "\n";
}
N=1
TIME: 00:00
MEMREAL: 786432

N=100
TIME: 00:00
MEMREAL: 786432

N=10000
TIME: 00:00
MEMREAL: 786432

N=1000000
TIME: 00:03
MEMREAL: 786432
jbboehr commented 11 years ago

The serialize calls are still leaking though.

woledzki commented 11 years ago

I will get more details in the evening - here's en example of generated class http://pastebin.com/CzsKbNqN and corresponding tpl http://pastebin.com/v15SayUr.

Class generated using tokenised template.

PS. Try bigger templates ~4k, I didn't have any problems with small ones.

woledzki commented 11 years ago

PHP 5.3.10

Your test with bigger template (http://pastebin.com/v15SayUr)

N=1
 TIME: 00:00
 MEMREAL: 5242880

N=100
 TIME: 00:00
 MEMREAL: 5242880

N=10000
 TIME: 00:00
 MEMREAL: 6291456

N=1000000
 TIME: 00:05
 MEMREAL: 270532608

N=5000000
 TIME: 00:22
 MEMREAL: 1591476224
jbboehr commented 11 years ago

I'm not getting any leaks with the latest version using the template you provided and some data I made.

Did you run all of?

git pull
git submodule update
make uninstall
make clean
make install

Script: http://pastebin.com/UVMpSqJg

N=10
TIME: 0.000155
MEMREAL: 786432

N=100
TIME: 0.001218
MEMREAL: 786432

N=1000
TIME: 0.019082
MEMREAL: 786432

N=10000
TIME: 0.122625
MEMREAL: 786432

N=100000
TIME: 1.295285
MEMREAL: 786432

N=1000000
TIME: 12.741253
MEMREAL: 786432
jbboehr commented 11 years ago

Here's my phpinfo php -v && php -i | grep -i "thread safety" PHP 5.3.10-1ubuntu3.6 with Suhosin-Patch (cli) (built: Mar 11 2013 14:31:48) Copyright (c) 1997-2012 The PHP Group Zend Engine v2.3.0, Copyright (c) 1998-2012 Zend Technologies with Xdebug v2.1.0, Copyright (c) 2002-2010, by Derick Rethans Thread Safety => disabled

jbboehr commented 11 years ago

Re-opening until I fix the memory leaks in the serializer

woledzki commented 11 years ago

:D sweet

N=1
 TIME: 00:00
 MEMREAL: 5242880

N=100
 TIME: 00:00
 MEMREAL: 5242880

N=10000
 TIME: 00:00
 MEMREAL: 5242880

N=1000000
 TIME: 00:04
 MEMREAL: 5242880

N=5000000
 TIME: 00:22
 MEMREAL: 5242880

I will stay with generating static classes on build time - makes more sense for me right now. Thanks for uber-fast tokenize() :)

jbboehr commented 11 years ago

Haha, no problem.

Technically, the mustache spec requires you to traverse up the context stack when a key is missing, so the generated code is not compatible with the spec, unfortunately. I had been thinking about adding a strict path option to disable that functionality.

Also, I'm not using a hash table (just a map) for the associative arrays, so that may cause performance issues on larger arrays. The other thing is I wrote the core code to work without using any PHP data structures (it's usable from C++), so it has to make a copy of all the data in order to not have any dependencies on PHP. The last thing is I'm using a lot of STL libraries, which are very safe to use, but are relatively very slow.

jbboehr commented 11 years ago

Memory leaks should be fixed in latest commit. Note that there were some API changes while implementing the new VM.