Immutable clases and op_arrays #3608

dstogov · 2018-10-15T18:52:40Z

No description provided.

dstogov · 2018-10-15T21:14:11Z

@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.

nikic · 2018-10-16T11:06:28Z

UPGRADING.INTERNALS

@@ -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.


nit: s/shard/shared

nikic · 2018-10-16T11:06:57Z

UPGRADING.INTERNALS

+     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.


nit: s/access/accessed

nikic · 2018-10-16T11:23:13Z

Zend/zend.c

+#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));


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);

I've introduced ZEND_MAP_PTR_IS_OFFSET(), ZEND_MAP_PTR_OFFSET2PTR() and ZEND_MAP_PTR_PTR2OFFSET() macros to encapsulate offset encoding details.

nikic · 2018-10-16T13:43:21Z

Zend/zend_execute.c

+		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);


Isn't this code already part of i_init_code_execute_data?

nikic · 2018-10-16T13:46:18Z

Zend/zend_inheritance.c

-		}
+	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));


The first part of the assert is redundant.

nikic · 2018-10-16T13:49:00Z

Zend/zend_inheritance.c

+			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);
+			}


Is this supposed to be in the above branch? This one is for user extends user. The previous one is user extends internal.

Or maybe both the above and the below (internal extends internal)?

Good catch. Thanks. Of course, this should be for both branches.

nikic · 2018-10-16T14:19:55Z

Zend/zend_interfaces.c

+
+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)) {


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).

You are right. Done.

nikic · 2018-10-16T14:24:49Z

Zend/zend_compile.h

+	ZEND_MAP_PTR_GET((op_array)->run_time_cache)
+
+#define ZEND_OP_ARRAY_EXTENSION(op_array, handle) \
+	RUN_TIME_CACHE(op_array)[handle]


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.

nikic · 2018-10-16T14:33:36Z

Zend/zend_map_ptr.h

+# define ZEND_MAP_PTR_DEF(type, name) \
+	type * ZEND_MAP_PTR(name)
+# define ZEND_MAP_PTR_GET(ptr) \
+	(*(ZEND_MAP_PTR(ptr)))


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.)

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.

Zend/zend_object_handlers.c

* 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

dstogov · 2018-10-17T12:56:09Z

Merged into master as a squash commit d57cd36

Immutable clases and op_arrays

9409958

dstogov force-pushed the immutable branch from 063a009 to 9409958 Compare October 15, 2018 20:37

nikic reviewed Oct 16, 2018

View reviewed changes

dstogov added 10 commits October 17, 2018 10:47

typo

9ef07c8

Hide offset encoding magic in ZEND_MAP_PTR_IS_OFFSET(), ZEND_MAP_PTR_…

8dadca8

…OFFSET2PTR() and ZEND_MAP_PTR_PTR2OFFSET() macros.

Removed duplicate code

d5a4108

Removed redundand assertion

b945548

Moved static class members initialization into the proper place.

c63fc5d

Added type cast

0276ea5

Added comment

ad7a78b

Reverted back ce->iterator_funcs_ptr. Initialize ce->iterator_funcs_p…

4740dab

…tr fields in immutable classes.

Merge branch 'master' into immutable

cd0c36c

* master: Remove the "auto" encoding Fixed bug #77025 Add vtbls for EUC-TW encoding

dstogov closed this Oct 17, 2018

dstogov deleted the immutable branch October 14, 2019 21:03

nielsdos mentioned this pull request Jan 8, 2023

Preloading anonymous class inheriting from autoloaded unlinked class leads to return type error #10131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Immutable clases and op_arrays #3608

Immutable clases and op_arrays #3608

dstogov commented Oct 15, 2018

dstogov commented Oct 15, 2018

nikic Oct 16, 2018

nikic Oct 16, 2018

nikic Oct 16, 2018

dstogov Oct 17, 2018

nikic Oct 16, 2018

nikic Oct 16, 2018

nikic Oct 16, 2018

nikic Oct 16, 2018

dstogov Oct 17, 2018

nikic Oct 16, 2018

dstogov Oct 17, 2018

nikic Oct 16, 2018

nikic Oct 16, 2018

dstogov Oct 17, 2018

dstogov commented Oct 17, 2018

Immutable clases and op_arrays #3608

Immutable clases and op_arrays #3608

Conversation

dstogov commented Oct 15, 2018

dstogov commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstogov commented Oct 17, 2018