Immutable clases and op_arrays #3608
Conversation
|
@nikic I've separated immutable classes/op_arrays and map_ptr implementation from #3538. I hope, I fixed all the known issues. I decided to start with more expensive map_ptr implementation even for non-ZTS build (it doesn't make limitation on special area size). Please take a look once again, I would like to merge this by the end of the week. |
UPGRADING.INTERNALS
Outdated
| @@ -98,6 +99,20 @@ PHP 7.4 INTERNALS UPGRADE NOTES | |||
| // ... | |||
| zend_release_properties(ht); | |||
|
|
|||
| g. Opcache may make classes and op_arrays immutable. Such classes are marked | |||
| by ZEND_ACC_IMMUTABLE flag, they are not going to be copied from opcache | |||
| shard memory to process memory and must not be modified at all. | |||
UPGRADING.INTERNALS
Outdated
| ZEND_MAP_PTR... abstraction macros and, basically, uses additional level of | ||
| indirection. op_array->run_time_cache, op_array->static_variables_ptr, | ||
| class_entry->static_members_table and class_entry->iterator_funcs_ptr now | ||
| have to be access through ZEND_MAP_PTR... macros. |
Zend/zend.c
Outdated
| #if ZEND_MAP_PTR_KIND == ZEND_MAP_PTR_KIND_PTR | ||
| return ptr; | ||
| #elif ZEND_MAP_PTR_KIND == ZEND_MAP_PTR_KIND_PTR_OR_OFFSET | ||
| return (void*)((CG(map_ptr_last) * sizeof(void*)) - (sizeof(void*) - 1)); |
There was a problem hiding this comment.
It took me a while to understand what this does. I think a more obvious way to write it would be
return (void*)((CG(map_ptr_last)-1) * sizeof(void*) + 1);
There was a problem hiding this comment.
I've introduced ZEND_MAP_PTR_IS_OFFSET(), ZEND_MAP_PTR_OFFSET2PTR() and ZEND_MAP_PTR_PTR2OFFSET() macros to encapsulate offset encoding details.
Zend/zend_execute.c
Outdated
| ZEND_MAP_PTR_INIT(op_array->run_time_cache, ptr); | ||
| ptr = (char*)ptr + sizeof(void*); | ||
| ZEND_MAP_PTR_SET(op_array->run_time_cache, ptr); | ||
| memset(ptr, 0, op_array->cache_size); |
There was a problem hiding this comment.
Isn't this code already part of i_init_code_execute_data?
Zend/zend_inheritance.c
Outdated
| } | ||
| if (ZEND_MAP_PTR(parent->iterator_funcs_ptr)) { | ||
| /* Must be initialized through iface->interface_gets_implemented() */ | ||
| ZEND_ASSERT(ZEND_MAP_PTR(ce->iterator_funcs_ptr) && ZEND_MAP_PTR_GET(ce->iterator_funcs_ptr)); |
There was a problem hiding this comment.
The first part of the assert is redundant.
Zend/zend_inheritance.c
Outdated
| if (UNEXPECTED(CE_STATIC_MEMBERS(parent_ce) == NULL)) { | ||
| ZEND_ASSERT(parent_ce->type == ZEND_INTERNAL_CLASS || (parent_ce->ce_flags & ZEND_ACC_IMMUTABLE)); | ||
| zend_class_init_statics(parent_ce); | ||
| } |
There was a problem hiding this comment.
Is this supposed to be in the above branch? This one is for user extends user. The previous one is user extends internal.
There was a problem hiding this comment.
Or maybe both the above and the below (internal extends internal)?
There was a problem hiding this comment.
Good catch. Thanks. Of course, this should be for both branches.
Zend/zend_interfaces.c
Outdated
|
|
||
| static zend_always_inline zend_class_iterator_funcs* zend_get_iterator_funcs_ptr(zend_class_entry *ce) /* {{{ */ | ||
| { | ||
| if (ZEND_MAP_PTR_GET(ce->iterator_funcs_ptr)) { |
There was a problem hiding this comment.
I don't really understand why iterator_funcs_ptr needs to make use of the MAP mechanism. Isn't all the necessary information always available at binding time? For immutable classes we should be able to compute this in opcache. It should also not be a problem for ZTS, as we don't reallocate methods (iirc).
Zend/zend_compile.h
Outdated
| ZEND_MAP_PTR_GET((op_array)->run_time_cache) | ||
|
|
||
| #define ZEND_OP_ARRAY_EXTENSION(op_array, handle) \ | ||
| RUN_TIME_CACHE(op_array)[handle] |
There was a problem hiding this comment.
At least in offset mode, RUN_TIME_CACHE(op_array) is a void* at this point, so it will not be possible to index it directly like this.
| # define ZEND_MAP_PTR_DEF(type, name) \ | ||
| type * ZEND_MAP_PTR(name) | ||
| # define ZEND_MAP_PTR_GET(ptr) \ | ||
| (*(ZEND_MAP_PTR(ptr))) |
There was a problem hiding this comment.
This should probably have a (void*) cast, to make the return type consistent with offset mode.
Or maybe the type should be passed to ZEND_MAP_PTR_GET as well. I don't see any standard way to automatically get the right type otherwise (the result of a ternary involving a non-null void pointer will be a void pointer, not the other pointer type, unfortunately.)
There was a problem hiding this comment.
I don't think these macros have to be compatible. Anyway, this is not a big issue, especially, while this MAP_PTR implementation is not used. We will able to add type cast later.
* master: Remove unused variable makefile_am_files Classify object handlers are required/optional Add support for getting SKIP_TAGSTART and SKIP_WHITE options Remove some obsolete config_vars.mk occurrences Remove bsd_converted from .gitignore Remove configuration parser and scanners ignores Remove obsolete buildconf.stamp from .gitignore [ci skip] Add magicdata.patch exception to .gitignore Remove outdated ext/spl/examples items from .gitignore Remove unused test.inc in ext/iconv/tests
…OFFSET2PTR() and ZEND_MAP_PTR_PTR2OFFSET() macros.
…tr fields in immutable classes.
* master: Remove the "auto" encoding Fixed bug #77025 Add vtbls for EUC-TW encoding
|
Merged into master as a squash commit d57cd36 |
No description provided.