Inheritance cache #6627

dstogov · 2021-01-20T15:12:01Z

No description provided.

dstogov · 2021-01-20T15:26:02Z

This is a PoC yet (some variance related tests fail). Inheritance cache should eliminate overhead of copying class data from SHM to process memory, run-time class linking, and then data deallocation. This also may help JIT generating better code for property access.

there are some problems with variance support
it would be great to eliminate limitation for ZEND_ACC_CONSTANTS_UPDATED and ZEND_ACC_PROPERTY_TYPES_RESOLVED to make all cached classes ZEND_ACC_IMMUTABLE
JIT might need additional class guards (not tested yet)

@nikic could you please review this and share your opinion and ideas for improvement.

nikic

I think this generally makes sense, I will need to look into the details a bit more.

it would be great to eliminate limitation for ZEND_ACC_CONSTANTS_UPDATED and ZEND_ACC_PROPERTY_TYPES_RESOLVED to make all cached classes ZEND_ACC_IMMUTABLE

Any ideas on how to do that? More map_ptr usage?

nikic · 2021-01-25T10:44:02Z

Zend/zend_execute_API.c

@@ -1059,7 +1059,15 @@ ZEND_API zend_class_entry *zend_lookup_class_ex(zend_string *name, zend_string *
 			if ((flags & ZEND_FETCH_CLASS_ALLOW_UNLINKED) ||
 				((flags & ZEND_FETCH_CLASS_ALLOW_NEARLY_LINKED) &&
 					(ce->ce_flags & ZEND_ACC_NEARLY_LINKED))) {
-				ce->ce_flags |= ZEND_ACC_HAS_UNLINKED_USES;
+				if (ce->ce_flags & ZEND_ACC_IMMUTABLE) {


If we need the hashtable for immutable classes, I think it would make sense to always use it rather than having two different mechanisms. It's a rare case, so being optimal is not so important.

Immutable class may be lazily loaded later, so the same hash key won't work.

nikic · 2021-01-25T10:49:55Z

ext/opcache/ZendAccelerator.c

+	}
+
+	if (ZCG(enabled) && accel_startup_ok) {
+		/* Override inheritance cache callbaks */


Suggested change

/* Override inheritance cache callbaks */

/* Override inheritance cache callbacks */

nikic · 2021-01-25T11:23:42Z

Zend/zend_inheritance.c

@@ -373,6 +376,11 @@ static inheritance_status zend_perform_covariant_class_type_check(
 		zend_class_entry *proto_scope, zend_type proto_type,
 		bool register_unresolved) {
 	bool have_unresolved = 0;
+
+	if (CG(current_linking_class)) {
+		CG(current_linking_class)->ce_flags |= ZEND_ACC_NEEDS_VARIANCE_CHECKS;


This also sets the flag if the

php-src/Zend/zend_inheritance.c

Line 410 in 442fe8f

if (zend_string_equals_ci(fe_class_name, proto_class_name)) {

case is hit (exactly the same name), which is very common. Maybe this should be inside the lookup_class function?

ZEND_ACC_NEEDS_VARIANCE_CHECKS is completely removed.

nikic · 2021-01-25T13:23:41Z

Did you already run any tests on how much difference this makes?

dstogov · 2021-01-26T09:25:10Z

Any ideas on how to do that? More map_ptr usage?

Yes, I already solved the typed property limitation and work on constants now.

dstogov · 2021-01-26T09:31:02Z

Did you already run any tests on how much difference this makes?

0.5% improvement on Wordpress (according to callgrind). I expect better improvement on heavy OOP apps. I'll test after fixing limitations (typed properties, AST constants, variance checks).

dstogov · 2021-02-05T06:45:30Z

@nikic I think, the PR is ready to merge. Please, review this and schedule more tests.

dstogov · 2021-02-05T09:48:12Z

I checked the performance improvement on https://github.com/phpbenchmarks/symfony (without JIT).

1000 requests takes 8% less time (one worker: REQUEST_METHOD=GET REQUEST_URI=/benchmark/helloworld SCRIPT_FILENAME=/home/dmitry/php/phpbenchmark/symfony/public/index.php REDIRECT_STATUS=200 SERVER_NAME=localhost SERVER_PORT=8080 SERVER_ADDR=127.0.0.1 REMOTE_PORT=40304 REMOTE_ADDR=127.0.0.1 sapi/cgi/php-cgi -d opcache.jit=0 -T 1000 > /dev/null)

For the same 1000 requests Callgrind shows 5% less executed instructions.
There are two main sources of performance improvement:

inheritance for each class is done once, so zend_inheritance.c takes 10 times less instructions
zend_accel_load_script() takes 3 times less instructions, because all classes are immutable.

Zend/zend.h

Zend/zend_compile.h

Ocramius · 2021-02-05T14:16:23Z

ext/opcache/tests/preload_unresolved_prop_type.phpt

 ===DONE===
--EXPECTF--
-Warning: Can't preload class Test with unresolved property types in %s on line %d
+--EXPECT--


This change in expectation seems weird: is the --FILE-- section mostly ignored and only here for representative purposes? Does the Warning still get emitted in some cases, after this patch?

Now opcache may preload classes with unresolved properties without problems.

Ocramius · 2021-02-05T14:17:08Z

ext/opcache/tests/preload_loadable_classes_3.phpt

 ?>
 --FILE--
-Unreachable
+<?php
+class Foo {


Related to ext/opcache/tests/preload_unresolved_prop_type.phpt - possibly --FILE-- section unused and only needed for representative purposes?

Ocramius · 2021-02-05T14:18:45Z

ext/opcache/tests/preload_loadable_classes_2.phpt

 ?>
 --FILE--
-Unreachable
+<?php


From a reader's perspective, this test is not really understandable: previously, it was clear that --FILE-- was not being used, but now it is, which makes the change in expected results quite suspicious

Previously, opcache couldn't preload classes with unresolved constants. Now this is not a problem. Constants that are not resolved during preloading, may be resolved later at run-time.

Ocramius · 2021-02-05T14:19:15Z

ext/opcache/tests/preload_012.phpt

@@ -11,5 +11,16 @@ require_once('skipif.inc');
 if (PHP_OS_FAMILY == 'Windows') die('skip Preloading is not supported on Windows');
 ?>
 --FILE--
--EXPECTF--
-Warning: Can't preload class Test with unresolved initializer for constant C in %s on line %d
+<?php


Same feedback as in ext/opcache/tests/preload_loadable_classes_2.phpt: would be better to keep --FILE-- as it was, then change only --EXPECTF--

We would have test with empty --FILE-- and empty --EXPECT-- . The new code shows, that now Test is preloaded, but resolution of constant "Test::C" failed, because "Foo::BAR" is not defined.

nikic · 2021-02-08T08:55:37Z

I triggered some additional tests, with the following new failures:

Some preload_* tests show a use-after-free under asan: https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=14598&view=ms.vss-test-web.build-test-results-tab&runId=330988&resultId=107298&paneView=debug
Many sapi/fpm tests assert with file cache (run run-tests with --file-cache-prime): https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=14598&view=ms.vss-test-web.build-test-results-tab&runId=330960&paneView=debug&resultId=115811

There is a use-after-free with Symfony preloading: https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=14598&view=logs&j=55323bc0-ded7-5cf3-3d38-be3edc0baeb7&t=b36a28df-cf17-5e74-fe6b-89f2999e3eee See

php-src/azure/community_job.yml

Lines 105 to 110 in 99a1bc0

    
                   php7.4 /usr/bin/composer create-project symfony/symfony-demo symfony_demo --no-progress 
        
                   cd symfony_demo 
        
                   export USE_ZEND_ALLOC=0 
        
                   export USE_TRACKED_ALLOC=1 
        
                   sed -i 's/PHP_SAPI/"cli-server"/g' var/cache/dev/App_KernelDevDebugContainer.preload.php 
        
                   php -d opcache.preload=var/cache/dev/App_KernelDevDebugContainer.preload.php public/index.php

for how to reproduce this part.

nikic · 2021-02-08T10:58:43Z

Zend/zend_types.h

 } zend_type;

 typedef struct {
 	uint32_t num_types;
 	zend_type types[1];
 } zend_type_list;

-#define _ZEND_TYPE_EXTRA_FLAGS_SHIFT 24
-#define _ZEND_TYPE_MASK ((1u << 24) - 1)
+#define _ZEND_TYPE_EXTRA_FLAGS_SHIFT 25


The intention here was that new flags are added at the low end, i.e. 1<<19. Types only go to 15, so this should be fine.

nikic · 2021-02-08T11:14:39Z

ext/opcache/ZendAccelerator.c

+					break;
+				}
+			}
+			if (entry->dependencies) {


This will try to check dependencies even if the above traits_and_interfaces check failed.

It would be best to extract the body of the while(entry) loop into a separate function so that early return can be used.

ext/opcache/ZendAccelerator.c

nikic · 2021-02-08T11:23:07Z

ext/opcache/ZendAccelerator.c

+							SEPARATE_ARRAY(zv);
+						} else if (Z_TYPE_P(zv) == IS_OBJECT || Z_TYPE_P(zv) == IS_RESOURCE) {
+							/* Can't cache */
+							return NULL;


Won't this miss something like array of objects?

nikic · 2021-02-08T11:35:23Z

ext/opcache/ZendAccelerator.c

+		entry->dependencies_count = zend_hash_num_elements(dependencies);
+		entry->dependencies = (zend_class_dependency*)ZCG(mem);
+		ZEND_HASH_FOREACH_STR_KEY_PTR(dependencies, dep_name, dep_ce) {
+			entry->dependencies[i].name = dep_name;


Are we guaranteed that dep_name is a permanent string here?

Class prototype, parent, and all interfaces and traits are IMMUTABLE. So they may refer only to strings in shared memory. Anyway, I'll add assertion.

@nikic all known test failures should be fixed now. Could you please reschedule tests.

I initiated full test run myself.

ext/opcache/ZendAccelerator.c

nikic · 2021-02-08T13:25:53Z

Zend/zend_inheritance.c

+				c = zend_arena_alloc(&CG(arena), sizeof(zend_class_constant));
+				memcpy(c, parent_const, sizeof(zend_class_constant));
+				parent_const = c;
+			}


I don't really get why this copy is needed.

We can't update constant value in shared memory, so we copy it into process memory.
If you comment this some tests are going to fail. For example tests/classes/constants_basic_006.phpt

nikic · 2021-02-08T13:36:42Z

MacOS build currently fails with:

/Users/runner/work/1/s/ext/opcache/zend_accelerator_util_funcs.c:313:32: error: unused function 'fast_memcpy' [-Werror,-Wunused-function]
static zend_always_inline void fast_memcpy(void *dest, const void *src, size_t size)

…erform any class compatibility checks during inheritance. It should be possible to reduce (or completely eliminate) this retriction.

…TY_TYPES_RESOLVED flag) to be immutable and to be used for inheritance caching.

dstogov · 2021-02-08T19:34:27Z

@nikic all known test failures should be fixed now. Could you please reschedule tests.
The only known problem - possible incorrect handling of "captured" static variables (your comment about arrays of objects).

…ror_message)

codecov-io · 2021-02-09T09:50:02Z

Codecov Report

Merging #6627 (cf2da7f) into master (042e0d8) will increase coverage by 0.04%.
The diff coverage is 83.31%.

@@            Coverage Diff             @@
##           master    #6627      +/-   ##
==========================================
+ Coverage   71.92%   71.96%   +0.04%     
==========================================
  Files         826      838      +12     
  Lines      316227   316737     +510     
==========================================
+ Hits       227439   227940     +501     
- Misses      88788    88797       +9

Impacted Files	Coverage Δ
Zend/zend.c	`89.09% <ø> (ø)`
Zend/zend_compile.h	`100.00% <ø> (ø)`
Zend/zend_types.h	`100.00% <ø> (ø)`
ext/curl/curl_private.h	`100.00% <ø> (ø)`
ext/opcache/shared_alloc_mmap.c	`82.05% <ø> (ø)`
ext/standard/crc32.c	`100.00% <ø> (ø)`
ext/standard/file.c	`91.33% <ø> (+0.14%)`	⬆️
ext/standard/streamsfuncs.c	`88.18% <ø> (+0.21%)`	⬆️
ext/zend_test/test_arginfo.h	`78.94% <0.00%> (-5.96%)`	⬇️
sapi/cli/php_cli_server.c	`0.00% <0.00%> (ø)`
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 882b418...cf2da7f. Read the comment docs.

dstogov · 2021-02-09T11:23:42Z

The following table compares Inheritance Cache and Preloading.
It shows time of 1000 requests to symphony_demo app without JIT (see azure/community_job.yml)
php-cgi -T10,1000 -d opcache.jit=0 public/index.php > /dev/null

	master	inheritance_cache
	20.87 sec	18.43 sec
preloding	17.14 sec	17.01 sec

As you can see, Inheritance Cache gives a part of Preloading speedup for free (Inheritance Cache - 12%, Preloading - 18%)

This reverts commit 2062e04.

…ed object, rsource or reference in static variable.

TysonAndre · 2021-02-09T19:22:01Z

I'm only moderately familiar with parts of opcache, so I'm not able to understand this as well as other reviewers:

What does this opcache code do for cyclic dependencies that would cause a runtime fatal error in php (E.g. trait A uses B, and trait B uses A)? (Same for cyclic dependencies in classes/interfaces, etc)

dstogov · 2021-02-09T19:50:31Z

I'm only moderately familiar with parts of opcache, so I'm not able to understand this as well as other reviewers:

What does this opcache code do for cyclic dependencies that would cause a runtime fatal error in php (E.g. trait A uses B, and trait B uses A)? (Same for cyclic dependencies in classes/interfaces, etc)

Inheritance Cache just can't store classes before they are linked by PHP, so PHP is going to fail as usually.

dstogov · 2021-02-09T19:57:16Z

Merged as 4b79dba

dstogov force-pushed the inheritance_cache branch 3 times, most recently from d75c5a6 to 442fe8f Compare January 25, 2021 09:05

nikic reviewed Jan 25, 2021

View reviewed changes

dstogov force-pushed the inheritance_cache branch 4 times, most recently from a81856a to 895976c Compare February 3, 2021 20:10

dstogov force-pushed the inheritance_cache branch from 1d698ff to 36c869d Compare February 4, 2021 23:58

dstogov requested a review from nikic February 5, 2021 06:43

Ocramius suggested changes Feb 5, 2021

View reviewed changes

nikic reviewed Feb 8, 2021

View reviewed changes

dstogov added 9 commits February 8, 2021 21:26

Inheritance cache PoC (incomplete)

d8c2afd

Fix incorrect alignment

db70dcc

Allow caching of user classes inherited from internal ones

ff426c9

Improve inheritance cache usage for early binding

ae0a3b3

Extend map_ptr buffer.

62ddc46

Support for variance. Inheritance cache is disaled for classes that p…

f53c613

…erform any class compatibility checks during inheritance. It should be possible to reduce (or completely eliminate) this retriction.

Initialize variable

c87dddb

Allow classes with unresolved property types (without ZEND_ACC_PROPER…

1d4a43e

…TY_TYPES_RESOLVED flag) to be immutable and to be used for inheritance caching.

Allow preloading classes with unresolved property types

db769ec

dstogov added 7 commits February 8, 2021 21:26

Removed deprecated initializer

eb8ad1e

Avoid dependencies verification if interfaces/traits are not the same

951ae6d

Add assertion

fce4f21

Fixed support for opcache.file_cache_only=1

32e3694

Prevent use-after-free

3a08917

Fixed incorrect closure handling

3609d17

ws

83a6238

dstogov force-pushed the inheritance_cache branch from 2bd659f to 83a6238 Compare February 8, 2021 18:27

Remove unused function

88c2c9b

dstogov added 5 commits February 8, 2021 22:47

Run full test matrix

2062e04

Prevent useless op_array duplication

1a0f7ea

Avoid optimization for non immutable op_arrays

69f8944

Fixed memory leaks with opcache.file_cache_only=1

b931e04

Delay CG(arena) deallocation, to avoid dangling pointer in PG(last_er…

cf2da7f

…ror_message)

dstogov added 4 commits February 9, 2021 16:51

Revert "Run full test matrix"

27203e2

This reverts commit 2062e04.

Added NEWS entry

0a8ae8b

typo

cd24004

Disable inheritance caching if some method of parent class has captur…

cff7765

…ed object, rsource or reference in static variable.

nikic approved these changes Feb 9, 2021

View reviewed changes

dstogov closed this Feb 9, 2021

iluuu1994 mentioned this pull request Jan 26, 2022

Undocumented change: order of returned parameters in get_object_vars() differs from previous versions #8003

Closed

zsistla mentioned this pull request Apr 4, 2022

All transactions unknown & 100% error rate post PHP8.1 upgrade (laravel v8) newrelic/newrelic-php-agent#387

Closed

iluuu1994 mentioned this pull request Aug 19, 2022

Fix opcache checksum calc for inheritance_cache #9377

Closed

greg0ire mentioned this pull request Jan 18, 2023

[Performance] Remove preloading? symfony/symfony-docs#17643

Open

Inheritance cache #6627

Inheritance cache #6627

dstogov commented Jan 20, 2021

dstogov commented Jan 20, 2021 •

edited

nikic left a comment

nikic Jan 25, 2021

dstogov Feb 5, 2021

nikic Jan 25, 2021

nikic Jan 25, 2021

dstogov Feb 5, 2021

nikic commented Jan 25, 2021

dstogov commented Jan 26, 2021

dstogov commented Jan 26, 2021

dstogov commented Feb 5, 2021

dstogov commented Feb 5, 2021

Ocramius Feb 5, 2021

dstogov Feb 8, 2021

Ocramius Feb 5, 2021

Ocramius Feb 5, 2021

dstogov Feb 8, 2021

Ocramius Feb 5, 2021

dstogov Feb 8, 2021

nikic commented Feb 8, 2021 •

edited

nikic Feb 8, 2021

nikic Feb 8, 2021

nikic Feb 8, 2021

nikic Feb 8, 2021

dstogov Feb 8, 2021

dstogov Feb 8, 2021

nikic Feb 8, 2021

dstogov Feb 8, 2021

nikic commented Feb 8, 2021

dstogov commented Feb 8, 2021

codecov-io commented Feb 9, 2021

dstogov commented Feb 9, 2021

TysonAndre commented Feb 9, 2021

dstogov commented Feb 9, 2021

dstogov commented Feb 9, 2021

	/* Override inheritance cache callbaks */
	/* Override inheritance cache callbacks */

Inheritance cache #6627

Inheritance cache #6627

Conversation

dstogov commented Jan 20, 2021

dstogov commented Jan 20, 2021 • edited

nikic left a 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

nikic commented Jan 25, 2021

dstogov commented Jan 26, 2021

dstogov commented Jan 26, 2021

dstogov commented Feb 5, 2021

dstogov commented Feb 5, 2021

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

nikic commented Feb 8, 2021 • edited

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

nikic commented Feb 8, 2021

dstogov commented Feb 8, 2021

codecov-io commented Feb 9, 2021

Codecov Report

dstogov commented Feb 9, 2021

TysonAndre commented Feb 9, 2021

dstogov commented Feb 9, 2021

dstogov commented Feb 9, 2021

dstogov commented Jan 20, 2021 •

edited

nikic commented Feb 8, 2021 •

edited