Use Zend API wherever possible

[sergeyklay]

Ported from: https://github.com/sergeyklay/issue1394/pull/6. Thanks to: @sjinks

---

Refs: #1392

[sergeyklay]

@sjinks Merged #1414 and #1411 into one branch

Also could you please take a look ffd356c. Just removed zephir_call_func_aparams_fast because it no longer used.

[sjinks]

Yes, sure - this is why I closed that PR and went to bed :-)

[sergeyklay]

@sjinks Lets continue here instead of example repo

[sjinks]

It looks like

test_globals->cache_enabled = 0;

solves the issue.

Back to the drawing board :-(

[sjinks]

If we disable cache slots, everything works.

diff --git a/kernels/ZendEngine3/fcall.c b/kernels/ZendEngine3/fcall.c
index 0e920e6..75fe800 100644
--- a/kernels/ZendEngine3/fcall.c
+++ b/kernels/ZendEngine3/fcall.c
@@ -206,15 +206,15 @@ int zephir_call_user_function(zval *object_pp, zend_class_entry *obj_ce, zephir_
        }

        if ((!cache_entry || !*cache_entry) && zephir_globals_ptr->cache_enabled) {
-               if (cache_slot > 0) {
-                       if (zephir_globals_ptr->scache[cache_slot]) {
-                               reload_cache = 0;
-                               temp_cache_entry = zephir_globals_ptr->scache[cache_slot];
-                               if (cache_entry) {
-                                       *cache_entry = temp_cache_entry;
-                               }
-                       }
-               }
+//             if (cache_slot > 0) {
+//                     if (zephir_globals_ptr->scache[cache_slot]) {
+//                             reload_cache = 0;
+//                             temp_cache_entry = zephir_globals_ptr->scache[cache_slot];
+//                             if (cache_entry) {
+//                                     *cache_entry = temp_cache_entry;
+//                             }
+//                     }
+//             }

                if (reload_cache) {
                        fcall_key_hash = zephir_make_fcall_key(&fcall_key, &fcall_key_len, (object_pp && type != zephir_fcall_ce ? Z_OBJCE_P(object_pp) : obj_ce), type, function_name);

[sjinks]

I would say the issue is in this part:

	if (cache_entry && *cache_entry) {
	/* We have a cache record, initialize scope */
		fcic.initialized   = 1;
		fcic.calling_scope = obj_ce;

		if (object_pp) {
			fcic.called_scope = Z_OBJCE_P(object_pp);
		}
		else {
			zend_class_entry *called_scope;
#if PHP_VERSION_ID >= 70100
			called_scope = zend_get_called_scope(EG(current_execute_data));
#else
			called_scope = EG(current_execute_data)->called_scope;
#endif

			if (obj_ce && (!called_scope || instanceof_function(called_scope, obj_ce))) {
				fcic.called_scope = obj_ce;
			}
			else {
				fcic.called_scope = called_scope;
			}
		}

		fcic.object = object_pp ? Z_OBJ_P(object_pp) : NULL;

#ifndef ZEPHIR_RELEASE
		fcic.function_handler = (*cache_entry)->f;
		++(*cache_entry)->times;
#else
		fcic.function_handler = *cache_entry;
#endif
	}

This basically corresponds to the following code from zend_call_method:

                zend_fcall_info_cache fcic;
                ZVAL_UNDEF(&fci.function_name); /* Unused */

                fcic.initialized = 1;
                if (!obj_ce) {
                        obj_ce = object ? Z_OBJCE_P(object) : NULL;
                }
                if (!fn_proxy || !*fn_proxy) {
                        HashTable *function_table = obj_ce ? &obj_ce->function_table : EG(function_table);
                        fcic.function_handler = zend_hash_str_find_ptr(
                                function_table, function_name, function_name_len);
                        if (fcic.function_handler == NULL) {
                                /* error at c-level */
                                zend_error_noreturn(E_CORE_ERROR, "Couldn't find implementation for method %s%s%s", obj_ce ? ZSTR_VAL(obj_ce->name) : "", obj_ce ? "::" : "", function_name);
                        }
                        if (fn_proxy) {
                                *fn_proxy = fcic.function_handler;
                        }
                } else {
                        fcic.function_handler = *fn_proxy;
                }

                fcic.calling_scope = obj_ce;
                if (object) {
                        fcic.called_scope = Z_OBJCE_P(object);
                } else {
                        zend_class_entry *called_scope = zend_get_called_scope(EG(current_execute_data));

                        if (obj_ce &&
                            (!called_scope ||
                             !instanceof_function(called_scope, obj_ce))) {
                                fcic.called_scope = obj_ce;
                        } else {
                                fcic.called_scope = called_scope;
                        }
                }
                fcic.object = object ? Z_OBJ_P(object) : NULL;

[sjinks]

With caching:

Test\Oo\OoDynamicA::getnew
Test\Oo\OoDynamicB::getnew

Without caching:

Test\Oo\OoDynamicA::call1 => self::call2 => self::getnew

[sjinks]

I have added the following debug code:

	fcic.initialized = 0;
	zval tmp;
	zephir_var_export_ex(&tmp, function_name);
	fprintf(stderr, "> %s called: %p, calling: %p, obj: %p\n", Z_STRVAL(tmp), fcic.called_scope, fcic.calling_scope, fcic.object);
	status = zend_call_function(&fci, &fcic);
	fprintf(stderr, "< called: %p, calling: %p, obj: %p\n", fcic.called_scope, fcic.calling_scope, fcic.object);

With fcic.initialized = 0 everything works as expected BUT i cannot understand why I have a different call sequence in this case.

With fcic.initialized = 0:

> array (
  0 => 'Test\\Oo\\OoDynamicA',
  1 => 'call1',
) called: 0x5572c4287290, calling: 0x5572c4287290, obj: (nil)
> array (
  0 => 'self',
  1 => 'call2',
) called: 0x5572c4287290, calling: 0x5572c4287290, obj: (nil)
> array (
  0 => 'self',
  1 => 'getnew',
) called: 0x5572c4287290, calling: 0x5572c4287290, obj: (nil)
< called: 0x5572c4287290, calling: 0x5572c4287290, obj: (nil)
< called: 0x5572c4287290, calling: 0x5572c4287290, obj: (nil)
< called: 0x5572c4287290, calling: 0x5572c4287290, obj: (nil)

Without:

< called: 0x56289892db10, calling: 0x56289892db10, obj: 0x7f708aaf1b60
> array (
  0 => 'Test\\Oo\\OoDynamicA',
  1 => 'getnew',
) called: 0x5628988c1290, calling: 0x5628988c1290, obj: (nil)
< called: 0x5628988c1290, calling: 0x5628988c1290, obj: (nil)
> array (
  0 => 'Test\\Oo\\OoDynamicB',
  1 => 'getnew',
) called: 0x562898909aa0, calling: 0x56289892e3b0, obj: (nil)
< called: 0x562898909aa0, calling: 0x56289892e3b0, obj: (nil)

[sjinks]

Test case:

$t = new \Test\Oo();
$obj9 = $t->testInstance9();
$this->assertTrue(is_object($obj9));
$this->assertInstanceOf('Test\Oo\OoDynamicA', $obj9);

$obj10 = $t->testInstance10();
$this->assertTrue(is_object($obj10));
$this->assertInstanceOf('Test\Oo\OoDynamicB', $obj10);

[sjinks]

GOOD:

> array (
  0 => 'Test\\Oo\\OoDynamicA',
  1 => 'getnew',
) called: 0x55c9846f5290, calling: 0x55c9846f5290, obj: (nil), h: 0x55c9846f5530
< called: 0x55c9846f5290, calling: 0x55c9846f5290, obj: (nil), h: 0x55c9846f5530
> array (
  0 => 'Test\\Oo\\OoDynamicB',
  1 => 'getnew',
) called: 0x55c98473daa0, calling: 0x55c9847623b0, obj: (nil), h: 0x55c9846f5530
< called: 0x55c9847623b0, calling: 0x55c9847623b0, obj: (nil), h: 0x55c984762720

BAD:

> array (
  0 => 'Test\\Oo\\OoDynamicA',
  1 => 'getnew',
) called: 0x561358edd290, calling: 0x561358edd290, obj: (nil), h: 0x561358edd530
< called: 0x561358edd290, calling: 0x561358edd290, obj: (nil), h: 0x561358edd530
> array (
  0 => 'Test\\Oo\\OoDynamicB',
  1 => 'getnew',
) called: 0x561358f25aa0, calling: 0x561358f4a3b0, obj: (nil), h: 0x561358edd530
< called: 0x561358f25aa0, calling: 0x561358f4a3b0, obj: (nil), h: 0x561358edd530

In the BAD case, the function handler for Test\\Oo\\OoDynamicA::getnew and Test\\Oo\\OoDynamicB::getnew is the same, which is a bug.

Since the bug manifests itself only in ZE3, it makes me think zephir_make_fcall_key is to blame, possibly:

	else if (type == zephir_fcall_static) {
#if PHP_VERSION_ID >= 70100
		calling_scope = zend_get_called_scope(EG(current_execute_data));
#else
		calling_scope = EG(current_execute_data)->called_scope;
#endif
		if (UNEXPECTED(!calling_scope)) {
			return 0;
		}
	}

[sergeyklay]

@sjinks I think we can skip optimization through caching right now for >= 70100

[sjinks]

Actually, this is not a bug in the Kernel, this is a bug in the translator.

PHP_METHOD(Test_Oo, testInstance9) {

        zval o;
        int ZEPHIR_LAST_CALL_STATUS;
        zephir_fcall_cache_entry *_0 = NULL;
        ZEPHIR_INIT_THIS();

        ZVAL_UNDEF(&o);

        ZEPHIR_MM_GROW();

        ZEPHIR_CALL_CE_STATIC(&o, test_oo_oodynamica_ce, "getnew", &_0, 48);
        zephir_check_call_status();
        RETURN_CCTOR(o);

}

PHP_METHOD(Test_Oo, testInstance10) {

        zval o;
        int ZEPHIR_LAST_CALL_STATUS;
        zephir_fcall_cache_entry *_0 = NULL;
        ZEPHIR_INIT_THIS();

        ZVAL_UNDEF(&o);

        ZEPHIR_MM_GROW();

        ZEPHIR_CALL_CE_STATIC(&o, test_oo_oodynamicb_ce, "getnew", &_0, 48);
        zephir_check_call_status();
        RETURN_CCTOR(o);

}

Both methods share the same cache slot (48), yet they are called from different contexts.

[sjinks]

@sergeyklay as far as I can tell, caching issue affects ZE3 (that is, PHP_VERSION_ID >= 70000) :-(

[sjinks]

Just tested ZE2: Test\Oo\OoDynamicA::getnew and Test\Oo\OoDynamicB::getnew do have the same function handler. In ZE3 this is no longer the case.

[sjinks]

obj_ce: Test\Oo\OoDynamicA
>  called: (nil), calling: (nil), obj: (nil), h: (nil), type=3
< called: 0x55c80e3f3e50, calling: 0x55c80e3f3e50, obj: (nil), h: 0x55c80e3f4100
obj_ce: Test\Oo\OoDynamicB
>  called: 0x55c80e492bb0, calling: 0x55c80e492bb0, obj: (nil), h: 0x55c80e3f4100, type=3
< called: 0x55c80e492bb0, calling: 0x55c80e492bb0, obj: (nil), h: 0x55c80e3f4100

[sergeyklay]

@sjinks As I can see we can keep caching for ZE2 and do not use for ZE3. At least for first time, to localize issue.

[sergeyklay]

@sjinks What do you think about disabling caching at all for ZE3?

[sjinks]

Probably we will have to do that :-(

I think I have found the place to fix in the compiler but I don't know its internals good enough

// StaticMethodCache::get()
        $mustBeCached = false;
        if (!$compilationContext->insideCycle) {
            if (!($method instanceof \ReflectionMethod)) {
                $classDefinition = $method->getClassDefinition();
                if (!$classDefinition->isBundled() && $allowNtsCache) {
                    $mustBeCached = true;
                } else {
                    if (!$method->isPrivate() && !$method->isFinal()) {
                        return 'NULL, 0';
                    }
                }
            } else {
                if (!$method->isPrivate() && !$method->isFinal()) {
                    return 'NULL, 0';
                }
            }
        }

The place where $mustBeCached is set to true: ideally we need to check whether the calling and the called classes are the same. However, $classDefinition->getName() returns OoDynamicA in both cases (understandable, as the method is defined in OoDynamicA), but $compilationContext->classDefinition->getName() returns Oo.

What I wanted to do was to find out if OoDynamicB is available somewhere (call context), and check whether call context matches $classDefinition->getName(). If it does not, do not set $mustBeCached.

[sjinks]

It is called from StaticCall::callFromClass. Looks like I need $classDefinition passed to it, but it is not passed to StaticMethodCache::get(). Беда-печаль :-(

[sergeyklay]

@sjinks Let me see what we can

[Jurigag]

Well after all changes it would be nice to at least test it on php7 with those changes performance if it's affected by any way.

[sjinks]

I think / hope we are good to go now :-)

[sergeyklay]

@sjinks Let me 15 min to provide some test

[Jurigag]

#1339 could you add some test for this?

[sergeyklay]

@Jurigag We’ll use for this a separated PR

[sergeyklay]

@sjinks Good work! Thank you 👍

[Jurigag]

Those changes are causing some pdo-related weird issues:

https://travis-ci.org/phalcon/cphalcon/jobs/214812313
https://travis-ci.org/phalcon/cphalcon/jobs/214812314

Like object with PDO connection is never destroyed - causing pdo connection to stay for all the time. Any idea? @sjinks

[sjinks]

Sqlite? Number of connections exceeded? Sounds weird...

[Jurigag]

On sqlite it's locked error - i guess the same problem like number of connections, on mysql it hits later this error on running codeception tests. It's exactly happening after this PR.

[Jurigag]

Let's go there - #1417