Implement apcu_inc/dec using atomic operations

This also changes apcu_inc/dec to wrap around on overflow, instead of saturating to a float value.
krakjoe · Oct 14, 2019 · 880ad0f · 880ad0f
1 parent ecd5957
commit 880ad0f
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 38 deletions.
diff --git a/apc_lock_api.h b/apc_lock_api.h
@@ -98,15 +98,18 @@ PHP_APCU_API void apc_lock_destroy(apc_lock_t *lock); /* }}} */
 # ifdef _WIN64
 #  define ATOMIC_INC(a) InterlockedIncrement64(&a)
 #  define ATOMIC_DEC(a) InterlockedDecrement64(&a)
+#  define ATOMIC_ADD(a, b) (InterlockedExchangeAdd64(&a, b) + b)
 #  define ATOMIC_CAS(a, old, new) (InterlockedCompareExchange64(&a, new, old) == old)
 # else
 #  define ATOMIC_INC(a) InterlockedIncrement(&a)
 #  define ATOMIC_DEC(a) InterlockedDecrement(&a)
+#  define ATOMIC_ADD(a, b) (InterlockedExchangeAdd(&a, b) + b)
 #  define ATOMIC_CAS(a, old, new) (InterlockedCompareExchange(&a, new, old) == old)
 # endif
 #else
 # define ATOMIC_INC(a) __sync_add_and_fetch(&a, 1)
 # define ATOMIC_DEC(a) __sync_sub_and_fetch(&a, 1)
+# define ATOMIC_ADD(a, b) __sync_add_and_fetch(&a, b)
 # define ATOMIC_CAS(a, old, new) __sync_bool_compare_and_swap(&a, old, new)
 #endif
 

diff --git a/php_apc.c b/php_apc.c
@@ -432,15 +432,15 @@ PHP_FUNCTION(apcu_sma_info)
 
 /* {{{ php_apc_update  */
 zend_bool php_apc_update(
-		zend_string *key, apc_cache_updater_t updater, void *data,
+		zend_string *key, apc_cache_atomic_updater_t updater, void *data,
 		zend_bool insert_if_not_found, time_t ttl)
 {
 	if (APCG(serializer_name)) {
 		/* Avoid race conditions between MINIT of apc and serializer exts like igbinary */
 		apc_cache_serializer(apc_user_cache, APCG(serializer_name));
 	}
 
-	return apc_cache_update(apc_user_cache, key, updater, data, insert_if_not_found, ttl);
+	return apc_cache_atomic_update_long(apc_user_cache, key, updater, data, insert_if_not_found, ttl);
 }
 /* }}} */
 
@@ -521,20 +521,14 @@ PHP_FUNCTION(apcu_add) {
 /* {{{ php_inc_updater */
 
 struct php_inc_updater_args {
-	zval step;
-	zval rval;
+	zend_long step;
+	zend_long rval;
 };
 
-static zend_bool php_inc_updater(apc_cache_t* cache, apc_cache_entry_t* entry, void* data) {
-	struct php_inc_updater_args *args = (struct php_inc_updater_args*) data;
-
-	if (Z_TYPE(entry->val) == IS_LONG) {
-		fast_long_add_function(&entry->val, &entry->val, &args->step);
-		ZVAL_COPY_VALUE(&args->rval, &entry->val);
-		return 1;
-	}
-
-	return 0;
+static zend_bool php_inc_updater(apc_cache_t *cache, zend_long *entry, void *data) {
+	struct php_inc_updater_args *args = (struct php_inc_updater_args *) data;
+	args->rval = ATOMIC_ADD(*entry, args->step);
+	return 1;
 }
 
 /* {{{ proto long apcu_inc(string key [, long step [, bool& success [, long ttl]]])
@@ -549,12 +543,12 @@ PHP_FUNCTION(apcu_inc) {
 		return;
 	}
 
-	ZVAL_LONG(&args.step, step);
+	args.step = step;
 	if (php_apc_update(key, php_inc_updater, &args, 1, ttl)) {
 		if (success) {
 			ZEND_TRY_ASSIGN_REF_TRUE(success);
 		}
-		RETURN_ZVAL(&args.rval, 0, 0);
+		RETURN_LONG(args.rval);
 	}
 
 	if (success) {
@@ -577,14 +571,13 @@ PHP_FUNCTION(apcu_dec) {
 		return;
 	}
 
-	ZVAL_LONG(&args.step, 0 - step);
-
+	args.step = 0 - step;
 	if (php_apc_update(key, php_inc_updater, &args, 1, ttl)) {
 		if (success) {
 			ZEND_TRY_ASSIGN_REF_TRUE(success);
 		}
 
-		RETURN_ZVAL(&args.rval, 0, 0);
+		RETURN_LONG(args.rval);
 	}
 
 	if (success) {

diff --git a/tests/apc_012.phpt b/tests/apc_012.phpt
@@ -1,36 +1,35 @@
 --TEST--
-APC: integer overflow consistency
+APC: Atomic inc + dec wrap around on overflow
 --SKIPIF--
 <?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
 --INI--
 apc.enabled=1
 apc.enable_cli=1
 --FILE--
 <?php
-$key="testkey";
-$i=PHP_INT_MAX;
-apcu_store($key, $i);
-var_dump($j=apcu_fetch($key));
-var_dump($i==$j);
+$key = "testkey";
 
-apcu_inc($key, 1);
-$i++;
-var_dump($j=apcu_fetch($key));
-var_dump($i==$j);
+apcu_store($key, PHP_INT_MAX);
+var_dump($i = apcu_inc($key, 1));
+var_dump($j = apcu_fetch($key));
+var_dump($i == $j);
+var_dump($j == PHP_INT_MIN);
+
+apcu_store($key, PHP_INT_MIN);
+var_dump($i = apcu_dec($key, 1));
+var_dump($j = apcu_fetch($key));
+var_dump($i == $j);
+var_dump($j == PHP_INT_MAX);
 
-$i=PHP_INT_MIN;
-apcu_store($key, $i);
-apcu_dec($key, 1);
-$i--;
-var_dump($j=apcu_fetch($key));
-var_dump($i==$j);
 ?>
 ===DONE===
 --EXPECTF--
-int(%d)
+int(%i)
+int(%i)
+bool(true)
 bool(true)
-float(%s)
+int(%i)
+int(%i)
 bool(true)
-float(%s)
 bool(true)
 ===DONE===