From b5e2331458a3f19d8613a519be34a18ae75282b0 Mon Sep 17 00:00:00 2001 From: Aaron Stone Date: Wed, 11 Apr 2018 06:54:49 -0700 Subject: [PATCH] First pass at using simple memcached_get when possible In binary mode, memcached_mget always issues two packets, which ends up making it slower than ascii mode for many users. This commit incorrectly believes that libmemcached supports UDP get, it does not, so that assumption needs to be reworked. Maybe UDP mode should automatically make both a TCP and a UDP connection? Or return a class with fewer methods so that it can be introspected or used with a magic method not defined to fall back to a TCP connection class as needed? --- php_memcached.c | 44 ++++++++++++++++++++++++---- tests/experimental/get_udp.phpt | 52 +++++++++++++++++++-------------- 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/php_memcached.c b/php_memcached.c index 930e1189..f783d68e 100644 --- a/php_memcached.c +++ b/php_memcached.c @@ -459,6 +459,9 @@ static static zend_bool s_memcached_result_to_zval(memcached_st *memc, memcached_result_st *result, zval *return_value); +static + zend_bool s_payload_to_zval(memcached_st *memc, const char *payload, size_t payload_len, uint32_t flags, zval *return_value); + static zend_string *s_zval_to_payload(php_memc_object_t *intern, zval *value, uint32_t *flags); @@ -1441,6 +1444,9 @@ void php_memc_get_impl(INTERNAL_FUNCTION_PARAMETERS, zend_bool by_key) zend_long get_flags = 0; zend_string *key; zend_string *server_key = NULL; + char *value = NULL; + uint32_t flags = 0; + size_t length = 0; zend_bool mget_status; memcached_return status = MEMCACHED_SUCCESS; zend_fcall_info fci = empty_fcall_info; @@ -1471,12 +1477,31 @@ void php_memc_get_impl(INTERNAL_FUNCTION_PARAMETERS, zend_bool by_key) MEMC_CHECK_KEY(intern, key); context.extended = (get_flags & MEMC_GET_EXTENDED); - context.return_value = return_value; - s_key_to_keys(&keys, key); - mget_status = php_memc_mget_apply(intern, server_key, &keys, s_get_apply_fn, context.extended, &context); - s_clear_keys(&keys); + // UDP and EXTENDED are not compatible + if (context.extended && memcached_behavior_get(intern->memc, MEMCACHED_BEHAVIOR_USE_UDP)) { + php_error_docref(NULL, E_WARNING, "Cannot use GET_EXTENDED in UDP mode"); + RETURN_FROM_GET; + } + + if (context.extended) { + mget_status = php_memc_mget_apply(intern, server_key, &keys, s_get_apply_fn, context.extended, &context); + } else { + printf("Using simple get\n"); + if (server_key) { + value = memcached_get_by_key(intern->memc, ZSTR_VAL(server_key), ZSTR_LEN(server_key), ZSTR_VAL(key), ZSTR_LEN(key), &length, &flags, &status); + } else { + value = memcached_get(intern->memc, ZSTR_VAL(key), ZSTR_LEN(key), &length, &flags, &status); + } + + mget_status = s_memc_status_handle_result_code(intern, status); + + if (value) { + mget_status |= s_payload_to_zval(intern->memc, value, length, flags, return_value); + free(value); + } + } if (!mget_status) { if (s_memc_status_has_result_code(intern, MEMCACHED_NOTFOUND) && fci.size > 0) { @@ -3681,16 +3706,23 @@ zend_bool s_unserialize_value (memcached_st *memc, int val_type, zend_string *pa static zend_bool s_memcached_result_to_zval(memcached_st *memc, memcached_result_st *result, zval *return_value) { - zend_string *data; const char *payload; size_t payload_len; uint32_t flags; - zend_bool retval = 1; payload = memcached_result_value(result); payload_len = memcached_result_length(result); flags = memcached_result_flags(result); + return s_payload_to_zval(memc, payload, payload_len, flags, return_value); +} + +static +zend_bool s_payload_to_zval(memcached_st *memc, const char *payload, size_t payload_len, uint32_t flags, zval *return_value) +{ + zend_string *data; + zend_bool retval = 1; + if (!payload && payload_len > 0) { php_error_docref(NULL, E_WARNING, "Could not handle non-existing value of length %zu", payload_len); return 0; diff --git a/tests/experimental/get_udp.phpt b/tests/experimental/get_udp.phpt index ad10a7cf..860987a3 100644 --- a/tests/experimental/get_udp.phpt +++ b/tests/experimental/get_udp.phpt @@ -4,42 +4,43 @@ Memcached::set()/delete() UDP --FILE-- setOption(Memcached::OPT_USE_UDP, true); -$m_udp->addServer('127.0.0.1', 11211, 1); +$m_udp->addServer("127.0.0.1", 11211, 1); +echo "\nGet incompatible\n"; +var_dump($m_udp->get("foo", null, Memcached::GET_EXTENDED)); +echo $m_udp->getResultMessage(), "\n"; +echo "\nDelete not found\n"; +$m_udp->delete("foo"); +var_dump($m_udp->delete("foo")); +echo $m_udp->getResultMessage(), "\n"; -error_reporting(0); +echo "\nSet\n"; +var_dump($m_udp->set("foo", "asdf", 10)); +echo $m_udp->getResultMessage(), "\n"; -echo "\n"; -echo "Delete not found\n"; -$m->delete('foo'); -var_dump($m_udp->delete('foo')); +echo "\nGet found\n"; +var_dump($m_udp->get("foo")); echo $m_udp->getResultMessage(), "\n"; -echo "\n"; -echo "Set\n"; -var_dump($m_udp->set('foo', "asdf", 10)); -sleep (1); +echo "\nDelete found\n"; +var_dump($m_udp->delete("foo")); +echo $m_udp->getResultMessage(), "\n"; +echo "\nGet not found\n"; +$m_udp->get("foo"); echo $m_udp->getResultMessage(), "\n"; -var_dump($m->get('foo')); -echo "\n"; -echo "Delete found\n"; -var_dump($m_udp->delete('foo')); -sleep (1); +echo "\nOK\n"; -echo $m_udp->getResultMessage(), "\n"; -$m->get('foo'); -echo $m->getResultMessage(), "\n"; +--EXPECTF-- +Get incompatible +Warning: Memcached::get(): Cannot use GET_EXTENDED in UDP mode in %s on line %d ---EXPECT-- Delete not found bool(true) SUCCESS @@ -47,9 +48,16 @@ SUCCESS Set bool(true) SUCCESS + +Get found string(4) "asdf" +SUCCESS Delete found bool(true) SUCCESS + +Get not found NOT FOUND + +OK