Reference dynamic functions through dynamic_defs #5595

nikic · 2020-05-18T14:43:11Z

Currently, dynamically declared functions and closures are inserted into the function table under a runtime definition key, and then later possibly renamed. When opcache is not used and a file containing a closure is repeatedly included, this leads to a very large memory leak, as the no longer needed closure declarations will never be freed (https://bugs.php.net/bug.php?id=76982).

With this patch, dynamic functions are instead stored in a dynamic_func_defs member on the op_array, which opcodes reference by index. When the parent op_array is destroyed, the dynamic_func_defs it contains are also destroyed (unless they are stilled used elsewhere, e.g. because they have been bound, or are used by a live closure). This resolves the fundamental part of the leak, though doesn't completely fix it yet due to some arena allocations.

The main non-obvious change here is to static variable handling: We can't destroy static_variables_ptr in destroy_op_array, as e.g. that would clear the static variables in a dynamic function when the op_array containing it is destroyed. Static variable destruction is separated out for this reason (we already do static variable destruction separately for normal functions, so we only need to handle main scripts).

nikic · 2021-02-23T08:59:48Z

@dstogov Can you please take a look at this?

dstogov

This is probably the right direction, but I'm not completely sure about all the implementation details.
Should this be extended to anonymous classes as well?

dstogov · 2021-02-24T08:01:08Z

Zend/zend_compile.h

+	/* Functions that are declared dynamically are stored here and
+	 * referenced by index from opcodes. */
+	zend_op_array **dynamic_func_defs;
+


Do we need "dynamic_func_defs" for each op array? or only for "main"?
May be we may store then in "literals"?

Do we need "dynamic_func_defs" for each op array? or only for "main"?

We need it for each op_array, as all op_arrays can contain closures or dynamically defined functions.

May be we may store then in "literals"?

Storing the functions in literals was my first attempt (#5593), but I found the implementation more complicated (though this may be due to unnecessary refcounted wrappers I added).

ext/opcache/zend_file_cache.c

dstogov · 2021-02-24T08:21:09Z

Zend/zend_closures.c

@@ -39,6 +39,7 @@ typedef struct _zend_closure {
 	zval              this_ptr;
 	zend_class_entry *called_scope;
 	zif_handler       orig_internal_handler;
+	HashTable        *static_variables;


I didn't completely understand, why do we need this new field. Just to reserve a new slot for map_ptr? And why we can't still use closure->func.op_array.static_variables?

We need to avoid overwriting the original static_variables here, so they can still be freed (if the closure has the last reference to the function).

I still didn't get this. We copy the whole op_array into zend_closure, so why can't we reuse closure->func.op_array.static_variables?
I replaced usage of closure->static_variables by closure->func.op_array.static_variables in your patch and don't see any tests failures (in Zend tests ext/opcache). Please demonstrate the failure.

I still didn't get this. We copy the whole op_array into zend_closure, so why can't we reuse closure->func.op_array.static_variables?

The issue is that while we copy the op_array itself, it points to shared resources (including static_variables). These are dropped when the op_array refcount reaches zero. If we reuse static_variables for a different purpose and refcount drop to zero in the closure dtor, then we will end up leaking the original static_variables.

I replaced usage of closure->static_variables by closure->func.op_array.static_variables in your patch and don't see any tests failures (in Zend tests ext/opcache). Please demonstrate the failure.

If I undo these changes using the following diff

diff --git a/Zend/zend_closures.c b/Zend/zend_closures.c index b3ce89a30d..5e05da46c7 100644 --- a/Zend/zend_closures.c +++ b/Zend/zend_closures.c @@ -39,7 +39,6 @@ typedef struct _zend_closure { zval this_ptr; zend_class_entry *called_scope; zif_handler orig_internal_handler; - HashTable *static_variables; } zend_closure; /* non-static since it needs to be referenced */ @@ -689,13 +688,11 @@ static void zend_create_closure_ex(zval *res, zend_function *func, zend_class_en /* For fake closures, we want to reuse the static variables of the original function. */ if (!is_fake) { if (closure->func.op_array.static_variables) { - HashTable *static_variables = - ZEND_MAP_PTR_GET(closure->func.op_array.static_variables_ptr); - closure->static_variables = zend_array_dup( - static_variables ? static_variables : closure->func.op_array.static_variables); + closure->func.op_array.static_variables = + zend_array_dup(closure->func.op_array.static_variables); } ZEND_MAP_PTR_INIT(closure->func.op_array.static_variables_ptr, - &closure->static_variables); + &closure->func.op_array.static_variables); } /* Runtime cache is scope-dependent, so we cannot reuse it if the scope changed */

then I get 30 failing tests on Zend/ due to memory leaks (debug, no opcache).

ah yes, I tested with opcache.

All the failures are memory leaks, because of missed deallocation of op_array.static_variables of dynamic functions, that is guarded by op_array->refcount.

I understood, how the patch works, but this looks a bit weird. We allocate new pointer to keep old/unused value of op_array.static_variables just to destroy it.

I think, it's more clear to explicitly deallocate static_variables of dynamic functions.

diff --git a/Zend/zend_opcode.c b/Zend/zend_opcode.c index ac9cc5f704..54edd806f2 100644 --- a/Zend/zend_opcode.c +++ b/Zend/zend_opcode.c @@ -600,6 +607,16 @@ ZEND_API void destroy_op_array(zend_op_array *op_array) if (op_array->static_variables) { zend_array_destroy(op_array->static_variables); } if (op_array->num_dynamic_func_defs) { for (i = 0; i < op_array->num_dynamic_func_defs; i++) { + if (op_array->dynamic_func_defs[i]->static_variables) { + zend_array_destroy(op_array->dynamic_func_defs[i]->static_variables); + op_array->dynamic_func_defs[i]->static_variables = NULL; + } destroy_op_array(op_array->dynamic_func_defs[i]); } efree(op_array->dynamic_func_defs); } } static void zend_update_extended_stmts(zend_op_array *op_array)

This patch fixes all the memory leaks for "Zend" tests. May be, it needs additional check for ZEND_ACC_CLOSURE.

I added an extra test to show the need for the ZEND_ACC_CLOSURE check in a9ff74f.

With that check, I think doing this is safe, because we know that no new closure can be created from the prototype function anymore after that point, so the static variables will not be needed anymore. I'm not completely sure this is the better approach.

Okay, I went with your suggestion.

nikic · 2021-02-24T08:39:37Z

Should this be extended to anonymous classes as well?

In principle yes, but anonymous classes have some additional problems: We don't increment/decrement class refcount used by objects, so we currently wouldn't know when the last use goes away. More generally, class references can also exist as strings (if e.g. get_class was called on an anonymous class object), and we probably aren't allowed to drop the class declaration if a string reference has escaped (which we again, don't track).

dstogov · 2021-02-24T10:43:21Z

It looks like the patch desn't solve the original problem. The following test (without opcahce) shows memory increase.

x.php

<?php
for ($i = 0; $i < 100; $i++) {
	for ($j =0; $j < 100; $j++) {
		include "y.php";
	}
	var_dump(memory_get_usage(0));
}

y.php

<?php
$f = function () {
	static $a = 42;
};

We already use <= for IS_SERIALIZED(), but the same general problem can also occur for IS_UNSERIALIZED(). We don't seem to hit this in practice prior to GH-5595 though.

nikic · 2021-02-24T10:52:27Z

It looks like the patch desn't solve the original problem. The following test (without opcahce) shows memory increase.

Yes, this doesn't fix the problem completely yet. As mentioned in the description, we still leak some memory due to arena allocations. Primarily because the op_array itself is arena allocated, so we always leak sizeof(zend_op_array). It will be necessary to move this off the arena to fix the remaining part of the leak.

nikic · 2021-03-01T08:43:36Z

@dstogov Any more thoughts on this one?

dstogov · 2021-03-01T09:21:50Z

I think, everything fine.

nikic mentioned this pull request May 18, 2020

Reference runtime-defined functions from literals #5593

Closed

2 tasks

nikic force-pushed the func-ref-2 branch 4 times, most recently from d1c4a5c to 04dd5b2 Compare May 19, 2020 10:35

nikic force-pushed the func-ref-2 branch 3 times, most recently from 329151e to fe74b99 Compare January 26, 2021 14:18

nikic force-pushed the func-ref-2 branch 4 times, most recently from c512a13 to fe47952 Compare February 22, 2021 16:05

nikic mentioned this pull request Feb 23, 2021

Don't use separate static variables in inherited methods #6719

Closed

dstogov reviewed Feb 24, 2021

View reviewed changes

nikic added 2 commits February 24, 2021 11:47

Reference dynamic functions through dynamic_defs

6dc79f7

Properly handle static variables

b08e977

nikic force-pushed the func-ref-2 branch from fe47952 to b0fe684 Compare February 24, 2021 10:53

Add additional test for static variables

a9ff74f

nikic force-pushed the func-ref-2 branch from b0fe684 to a9ff74f Compare February 24, 2021 13:51

Explicitly destroy closure static_variables

01e1f2a

php-pulls closed this in 47a2e5c Mar 1, 2021

This was referenced Jul 23, 2021

Memory leaks in PHPUnit laravel/framework#30736

Closed

PHPUnit 8 consumes much more memory comparing PHPUnit 7 sebastianbergmann/phpunit#3915

Closed

Reference dynamic functions through dynamic_defs #5595

Reference dynamic functions through dynamic_defs #5595

Uh oh!

Conversation

nikic commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Feb 23, 2021

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Feb 24, 2021

Uh oh!

dstogov commented Feb 24, 2021

Uh oh!

nikic commented Feb 24, 2021

Uh oh!

nikic commented Mar 1, 2021

Uh oh!

dstogov commented Mar 1, 2021

Uh oh!

Uh oh!

nikic commented May 18, 2020 •

edited

Loading