Optimize SplObjectStorage native read/write dimension handlers

TysonAndre · TysonAndre · commit c17ee70a964f · 2021-11-27T08:14:56.000-05:00
This makes reading/writing with `$splObjectStorage[$offset]` shorthand twice as fast as it was previously, and a bit faster than offsetGet/offsetSet instead of (previously) much slower. Related to phpGH-7690
diff --git a/ext/spl/spl_observer.c b/ext/spl/spl_observer.c
@@ -44,13 +44,19 @@ PHPAPI zend_class_entry     *spl_ce_MultipleIterator;
 
 PHPAPI zend_object_handlers spl_handler_SplObjectStorage;
 
+typedef struct _spl_SplObjectStorage_fptrs {
+	zend_function    *get_hash;
+	bool              override_read_dimension;
+	bool              override_write_dimension;
+} spl_SplObjectStorage_fptrs;
+
 typedef struct _spl_SplObjectStorage { /* {{{ */
-	HashTable         storage;
-	zend_long         index;
-	HashPosition      pos;
-	zend_long         flags;
-	zend_function    *fptr_get_hash;
-	zend_object       std;
+	HashTable                   storage;
+	zend_long                   index;
+	HashPosition                pos;
+	zend_long                   flags;
+	spl_SplObjectStorage_fptrs *fptrs;
+	zend_object                 std;
 } spl_SplObjectStorage; /* }}} */
 
 /* {{{ storage is an assoc array of [zend_object*]=>[zval *obj, zval *inf] */
@@ -73,15 +79,19 @@ void spl_SplObjectStorage_free_storage(zend_object *object) /* {{{ */
 	zend_object_std_dtor(&intern->std);
 
 	zend_hash_destroy(&intern->storage);
+
+	if (UNEXPECTED(intern->fptrs)) {
+		efree_size(intern->fptrs, sizeof(spl_SplObjectStorage_fptrs));
+	}
 } /* }}} */
 
 static int spl_object_storage_get_hash(zend_hash_key *key, spl_SplObjectStorage *intern, zend_object *obj) {
-	if (intern->fptr_get_hash) {
+	if (UNEXPECTED(intern->fptrs) && intern->fptrs->get_hash) {
 		zval param;
 		zval rv;
 		ZVAL_OBJ(&param, obj);
 		zend_call_method_with_1_params(
-			&intern->std, intern->std.ce, &intern->fptr_get_hash, "getHash", &rv, &param);
+			&intern->std, intern->std.ce, &intern->fptrs->get_hash, "getHash", &rv, &param);
 		if (!Z_ISUNDEF(rv)) {
 			if (Z_TYPE(rv) == IS_STRING) {
 				key->key = Z_STR(rv);
@@ -125,8 +135,52 @@ static spl_SplObjectStorageElement* spl_object_storage_get(spl_SplObjectStorage
 	}
 } /* }}} */
 
+static spl_SplObjectStorageElement *spl_object_storage_create_element(zend_object *obj, zval *inf) /* {{{ */
+{
+	spl_SplObjectStorageElement *pelement = emalloc(sizeof(spl_SplObjectStorageElement));
+	pelement->obj = obj;
+	GC_ADDREF(obj);
+	if (inf) {
+		ZVAL_COPY(&pelement->inf, inf);
+	} else {
+		ZVAL_NULL(&pelement->inf);
+	}
+	return pelement;
+} /* }}} */
+
+static spl_SplObjectStorageElement *spl_object_storage_attach_handle(spl_SplObjectStorage *intern, zend_object *obj, zval *inf) /* {{{ */
+{
+	uint32_t handle = obj->handle;
+	zval *entry_zv;
+	spl_SplObjectStorageElement *pelement;
+	entry_zv = zend_hash_index_lookup(&intern->storage, handle);
+	ZEND_ASSERT(intern->fptrs == NULL || !intern->fptrs->override_write_dimension);
+
+	if (Z_TYPE_P(entry_zv) != IS_NULL) {
+		ZEND_ASSERT(Z_TYPE_P(entry_zv) == IS_PTR);
+		pelement = Z_PTR_P(entry_zv);
+		/* FIXME unsafe if destructor of inf moves/removes this entry */
+		zval_ptr_dtor(&pelement->inf);
+		if (inf) {
+			ZVAL_COPY(&pelement->inf, inf);
+		} else {
+			ZVAL_NULL(&pelement->inf);
+		}
+		return pelement;
+	}
+
+	ZEND_ASSERT((GC_FLAGS(&intern->storage) & IS_ARRAY_PERSISTENT) == 0);
+	pelement = spl_object_storage_create_element(obj, inf);
+	ZVAL_PTR(entry_zv, pelement);
+	return pelement;
+} /* }}} */
+
 spl_SplObjectStorageElement *spl_object_storage_attach(spl_SplObjectStorage *intern, zend_object *obj, zval *inf) /* {{{ */
 {
+	if (EXPECTED(intern->fptrs == NULL || !intern->fptrs->override_write_dimension)) {
+		return spl_object_storage_attach_handle(intern, obj, inf);
+	}
+
 	spl_SplObjectStorageElement *pelement, element;
 	zend_hash_key key;
 	if (spl_object_storage_get_hash(&key, intern, obj) == FAILURE) {
@@ -136,6 +190,7 @@ spl_SplObjectStorageElement *spl_object_storage_attach(spl_SplObjectStorage *int
 	pelement = spl_object_storage_get(intern, &key);
 
 	if (pelement) {
+		/* FIXME unsafe if destructor of inf moves/removes this entry */
 		zval_ptr_dtor(&pelement->inf);
 		if (inf) {
 			ZVAL_COPY(&pelement->inf, inf);
@@ -189,6 +244,9 @@ void spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjectStorag
 	intern->index = 0;
 } /* }}} */
 
+#define SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, method) \
+	(((zend_function *)zend_hash_str_find_ptr(&(class_type)->function_table, (method), sizeof((method)) - 1))->common.scope != spl_ce_SplObjectStorage)
+
 static zend_object *spl_object_storage_new_ex(zend_class_entry *class_type, zend_object *orig) /* {{{ */
 {
 	spl_SplObjectStorage *intern;
@@ -207,10 +265,21 @@ static zend_object *spl_object_storage_new_ex(zend_class_entry *class_type, zend
 
 	while (parent) {
 		if (parent == spl_ce_SplObjectStorage) {
+			/* TODO: Make these singletons with a map from class entry to IS_NULL/IS_PTR */
 			if (class_type != spl_ce_SplObjectStorage) {
-				intern->fptr_get_hash = zend_hash_str_find_ptr(&class_type->function_table, "gethash", sizeof("gethash") - 1);
-				if (intern->fptr_get_hash->common.scope == spl_ce_SplObjectStorage) {
-					intern->fptr_get_hash = NULL;
+				spl_SplObjectStorage_fptrs fptrs;
+				fptrs.get_hash = zend_hash_str_find_ptr(&class_type->function_table, "gethash", sizeof("gethash") - 1);
+				if (fptrs.get_hash->common.scope == spl_ce_SplObjectStorage) {
+					fptrs.get_hash = NULL;
+				}
+				fptrs.override_read_dimension = fptrs.get_hash != NULL ||
+					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, "offsetget") ||
+					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, "offsetexists");
+
+				fptrs.override_write_dimension = fptrs.get_hash != NULL || SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, "offsetset");
+				if (fptrs.get_hash || fptrs.override_read_dimension || fptrs.override_write_dimension) {
+					intern->fptrs = emalloc(sizeof(spl_SplObjectStorage_fptrs));
+					*intern->fptrs = fptrs;
 				}
 			}
 			break;
@@ -361,6 +430,39 @@ PHP_METHOD(SplObjectStorage, attach)
 	spl_object_storage_attach(intern, obj, inf);
 } /* }}} */
 
+static zval *spl_object_storage_read_dimension(zend_object *object, zval *offset, int type, zval *rv)
+{
+	spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
+	if (UNEXPECTED(offset == NULL || Z_TYPE_P(offset) != IS_OBJECT || (intern->fptrs && intern->fptrs->override_read_dimension))) {
+		/* Can't optimize it if getHash, offsetExists, or offsetGet is overridden */
+		return zend_std_read_dimension(object, offset, type, rv);
+	}
+	spl_SplObjectStorageElement *element = zend_hash_index_find_ptr(&intern->storage, Z_OBJ_HANDLE_P(offset));
+	if (type == BP_VAR_IS) {
+		if (element == NULL) {
+			return &EG(uninitialized_zval);
+		}
+	}
+
+	if (!element) {
+		zend_throw_exception_ex(spl_ce_UnexpectedValueException, 0, "Object not found");
+		return NULL;
+	} else {
+		ZVAL_COPY_DEREF(rv, &element->inf);
+		return rv;
+	}
+}
+
+static void spl_object_storage_write_dimension(zend_object *object, zval *offset, zval *inf)
+{
+	spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
+	if (UNEXPECTED(offset == NULL || Z_TYPE_P(offset) != IS_OBJECT || (intern->fptrs && intern->fptrs->override_write_dimension))) {
+		zend_std_write_dimension(object, offset, inf);
+		return;
+	}
+	spl_object_storage_attach_handle(intern, Z_OBJ_P(offset), inf);
+}
+
 /* {{{ Detaches an object from the storage */
 PHP_METHOD(SplObjectStorage, detach)
 {
@@ -1201,6 +1303,8 @@ PHP_MINIT_FUNCTION(spl_observer)
 	spl_handler_SplObjectStorage.clone_obj       = spl_object_storage_clone;
 	spl_handler_SplObjectStorage.get_gc          = spl_object_storage_get_gc;
 	spl_handler_SplObjectStorage.free_obj        = spl_SplObjectStorage_free_storage;
+	spl_handler_SplObjectStorage.read_dimension  = spl_object_storage_read_dimension;
+	spl_handler_SplObjectStorage.write_dimension = spl_object_storage_write_dimension;
 
 	spl_ce_MultipleIterator = register_class_MultipleIterator(zend_ce_iterator);
 	spl_ce_MultipleIterator->create_object = spl_SplObjectStorage_new;
diff --git a/ext/spl/tests/SplObjectStorage_coalesce.phpt b/ext/spl/tests/SplObjectStorage_coalesce.phpt
@@ -0,0 +1,68 @@
+--TEST--
+SplObjectStorage magic operators
+--FILE--
+<?php
+$i = 'dynamic';
+$v = $i . '_';
+$s = new SplObjectStorage();
+$o1 = new Stdclass;
+$o2 = new Stdclass;
+var_dump($s[$o1] ?? 'default');
+var_dump($s[$o1] ??= $i);
+var_dump($s[$o1] ??= null);
+var_dump($s[$o1] ??= false);
+var_dump($s[$o1] ?? $i);
+$s[$o2] = null;
+var_dump(isset($s[$o2]));
+var_dump($s[$o2] ?? new stdClass());
+var_dump(isset($s[$o2]));
+try {
+    $s['invalid'] = 123;
+} catch (Error $e) {
+    printf("%s: %s\n", $e::class, $e->getMessage());
+}
+try {
+    var_dump(isset($s['invalid']));
+} catch (Error $e) {
+    printf("%s: %s\n", $e::class, $e->getMessage());
+}
+$a = &$s[$o1];
+
+var_dump($s);
+
+?>
+--EXPECTF--
+string(7) "default"
+string(7) "dynamic"
+string(7) "dynamic"
+string(7) "dynamic"
+string(7) "dynamic"
+bool(true)
+object(stdClass)#4 (0) {
+}
+bool(true)
+TypeError: SplObjectStorage::offsetSet(): Argument #1 ($object) must be of type object, string given
+TypeError: SplObjectStorage::offsetExists(): Argument #1 ($object) must be of type object, string given
+
+Notice: Indirect modification of overloaded element of SplObjectStorage has no effect in %s on line 26
+object(SplObjectStorage)#1 (1) {
+  ["storage":"SplObjectStorage":private]=>
+  array(2) {
+    [0]=>
+    array(2) {
+      ["obj"]=>
+      object(stdClass)#2 (0) {
+      }
+      ["inf"]=>
+      string(7) "dynamic"
+    }
+    [1]=>
+    array(2) {
+      ["obj"]=>
+      object(stdClass)#3 (0) {
+      }
+      ["inf"]=>
+      NULL
+    }
+  }
+}
