Merge lp:~posulliv/libmemcached/c++-interface into lp:~tangent-org/libmemcached/trunk

[Padraig O'Sullivan]

[Padraig O'Sullivan (posulliv)]

So I was thinking that when we actually port the memcached UDF's to Drizzle, we should use the libmemcached C++ interface since Drizzle is a C++ project. Thus, to enable that, I did some refactoring of the libmemcached C++ interface wrapper.

Basically, all I did was the following:

* replace parameters as char * with references to std::string
* use std::vector in places where arrays of pointers were being passed
* modified the return types of numerous API function to be bool instead of memcached_return
* added more test cases to the C++ API test file

I'm just proposing this for merging to get some feedback on the approach I've taken. I can easily keep the existing interface by just overloading those API functions if that is wanted.

-Padraig

[Brian Aker (brianaker)]

Thanks!

This is a huge improvement over the current interface (which I am sure no one is using).

[Brian Aker (brianaker)]

Looking through the code.. mget() going to be quite slow I suspect because of the memory allocation that will be done because of it.

I modified mget() to not do a malloc, and instead use a vector for the key sizes.

I suspect we could look at the lower level interfaces and find a way of getting the advantages of mget() without doing the vector overhead. Probably just a matter of flipping the bit for pipelining requests and sending down directly the bits from the std:string.

Code is now in trunk.

=== modified file 'libmemcached/memcached.hh'
--- libmemcached/memcached.hh	2009-07-07 21:57:24 +0000
+++ libmemcached/memcached.hh	2009-07-11 19:10:39 +0000
@@ -1,175 +1,354 @@
-#include "libmemcached/memcached.h"
-#include <string.h>
-#include <stdio.h>
+#include <libmemcached/memcached.h>
+
+#include <string>
+#include <vector>
 
 class Memcached
 {
-  memcached_st memc;
-  memcached_result_st result;
-
 public:
 
-  Memcached() : memc(), result()
+  Memcached() 
+    : 
+      memc(),
+      result()
   {
     memcached_create(&memc);
   }
 
-  Memcached(memcached_st *clone) : memc(), result()
+  Memcached(memcached_st *clone) 
+    : 
+      memc(),
+      result()
   {
     memcached_clone(&memc, clone);
   }
-  char *fetch (char *key, size_t *key_length, size_t *value_length)
-  {
-    uint32_t flags;
-    memcached_return rc;
-
-    return memcached_fetch(&memc, key, key_length,
-                    value_length, &flags, &rc);
-  }
-  char *get(const char *key, size_t *value_length)
-  {
-    uint32_t flags;
-    memcached_return rc;
-
-    return memcached_get(&memc, key, strlen(key),
-                         value_length, &flags, &rc);
-  }
-
-  char *get_by_key(const char *master_key, const char *key, 
-                   size_t *value_length)
-  {
-    uint32_t flags;
-    memcached_return rc;
-
-    return memcached_get_by_key(&memc, master_key, strlen(master_key), 
-                                key, strlen(key),
-                                value_length, &flags, &rc);
-  }
-
-  memcached_return mget(char **keys, size_t *key_length, 
-                        unsigned int number_of_keys)
-  {
-
-    return memcached_mget(&memc, keys, key_length, number_of_keys);
-  }
-
-  memcached_return set(const char *key, const char *value, size_t value_length)
-  {
-    return memcached_set(&memc, key, strlen(key),
-                         value, value_length,
-                         time_t(0), uint32_t(0));
-  }
-
-  memcached_return set_by_key(const char *master_key, const char *key, 
-                              const char *value, size_t value_length)
-  {
-    return memcached_set_by_key(&memc, master_key, strlen(master_key),
-                         key, strlen(key),
-                         value, value_length,
-                         time_t(0),
-                         uint32_t(0) );
-  }
-  memcached_return
-    increment(const char *key, unsigned int offset, uint64_t *value)
-  {
-    return memcached_increment(&memc, key, strlen(key),
-                         offset, value);
-  }
-  memcached_return
-    decrement(const char *key, unsigned int offset, uint64_t *value)
-  {
-    return memcached_decrement(&memc, key, strlen(key),
-                         offset, value);
-  }
-
-
-  memcached_return add(const char *key, const char *value, size_t value_length)
-  {
-    return memcached_add(&memc, key, strlen(key), value, value_length, 0, 0);
-  }
-  memcached_return add_by_key(const char *master_key, const char *key, 
-                              const char *value, size_t value_length)
-  {
-    return memcached_add_by_key(&memc, master_key, strlen(master_key),
-                                key, strlen(key),
-                                value, value_length,
-                                0, 0);
-  }
-
-  memcached_return replace(const char *key, const char *value, 
-                           size_t value_length)
-  {
-    return memcached_replace(&memc, key, strlen(key),
-                     value, value_length,
-                     0, 0);
-  }
-  memcached_return replace_by_key(const char *master_key, const char *key, 
-                                  const char *value, size_t value_length)
-  {
-    return memcached_replace_by_key(&memc, master_key, strlen(master_key),
-                                    key, strlen(key),
-                                    value, value_length, 0, 0);
-  }
-
-  memcached_return prepend(const char *key, const char *value, 
-                           size_t value_length)
-  {
-    return memcached_prepend(&memc, key, strlen(key),
-                    value, value_length, 0, 0);
-  }
-  memcached_return prepend_by_key(const char *master_key, const char *key, 
-                                  const char *value, size_t value_length)
-  {
-    return memcached_prepend_by_key(&memc, master_key, strlen(master_key),
-                                    key, strlen(key),
-                                    value, value_length,
-                                    0,
-                                    0);
-  }
-
-  memcached_return  append(const char *key, const char *value, 
-                           size_t value_length)
-  {
-    return memcached_append(&memc, key, strlen(key),
-                    value, value_length, 0, 0);
-  }
-  memcached_return  append_by_key(const char *master_key, const char *key, 
-                                  const char *value, size_t value_length)
-  {
-    return memcached_append_by_key(&memc,
-                                   master_key, strlen(master_key),
-                                   key, strlen(key),
-                                   value, value_length, 0, 0);
-  }
-  memcached_return  cas(const char *key, const char *value, 
-                        size_t value_length, uint64_t cas_arg)
-  {
-    return memcached_cas(&memc, key, strlen(key),
-                    value, value_length, 0, 0, cas_arg);
-  }
-  memcached_return  cas_by_key(const char *master_key, const char *key, 
-                               const char *value, size_t value_length, 
-                               uint64_t cas_arg)
-  {
-    return memcached_cas_by_key(&memc,
-                                master_key, strlen(master_key),
-                                key, strlen(key),
-                                value, value_length,
-                                0, 0, cas_arg);
-  }
-  // using 'remove' vs. 'delete' since 'delete' is a keyword 
-  memcached_return remove(const char *key)
-  {
-    return memcached_delete (&memc, key, strlen(key), 0);
-
-  }
-  memcached_return delete_by_key(const char *master_key, const char *key)
-  {
-    return memcached_delete_by_key(&memc, master_key, strlen(master_key),
-                           key, strlen(key), 0);
-  }
+
   ~Memcached()
   {
     memcached_free(&memc);
   }
+
+  bool fetch(std::string &key, 
+             std::string &ret_val,
+             size_t *key_length, 
+             size_t *value_length,
+             uint32_t *flags,
+             memcached_return *rc)
+  {
+    char ret_key[MEMCACHED_MAX_KEY];
+    char *value= memcached_fetch(&memc, ret_key, key_length,
+                                 value_length, flags, rc);
+    if (value)
+    {
+      ret_val.assign(value);
+      key.assign(ret_key);
+      return true;
+    }
+    return false;
+  }
+
+  std::string get(const std::string &key, size_t *value_length) 
+  {
+    uint32_t flags;
+    memcached_return rc;
+    std::string ret_val;
+
+    char *value= memcached_get(&memc, key.c_str(), key.length(),
+                               value_length, &flags, &rc);
+    if (value)
+    {
+      ret_val.assign(value);
+    }
+    return ret_val;
+  }
+
+  std::string get_by_key(const std::string &master_key, 
+                         const std::string &key, 
+                         size_t *value_length)
+  {
+    uint32_t flags;
+    memcached_return rc;
+    std::string ret_val;
+
+    char *value= memcached_get_by_key(&memc, master_key.c_str(), master_key.length(), 
+                                      key.c_str(), key.length(),
+                                      value_length, &flags, &rc);
+    if (value)
+    {
+      ret_val.assign(value);
+    }
+    return ret_val;
+  }
+
+  bool mget(std::vector<std::string> &keys)
+  {
+    /*
+     * Construct an array which will contain the length
+     * of each of the strings in the input vector. Also, to
+     * interface with the memcached C API, we need to convert
+     * the vector of std::string's to a vector of char *.
+     */
+    size_t *key_len= static_cast<size_t *>(malloc(keys.size() * sizeof(size_t)));
+    if (key_len == NULL)
+    {
+      return false;
+    }
+    std::vector<char *> real_keys;
+    std::vector<std::string>::iterator it= keys.begin();
+    int i= 0;
+    while (it != keys.end())
+    {
+      real_keys.push_back(const_cast<char *>((*it).c_str()));
+      key_len[i++]= (*it).length();
+      ++it;
+    }
+
+    /*
+     * If the std::vector of keys is empty then we cannot 
+     * call memcached_mget as we will get undefined behavior.
+     */
+    if (!real_keys.empty())
+    {
+      memcached_return rc= memcached_mget(&memc, &real_keys[0], key_len, 
+                                          static_cast<unsigned int>(real_keys.size()));
+      return (rc == MEMCACHED_SUCCESS);
+    }
+
+    return false;
+  }
+
+  bool set(const std::string &key, 
+           const std::string &value,
+           time_t expiration,
+           uint32_t flags)
+  {
+    memcached_return rc= memcached_set(&memc, 
+                                       key.c_str(), key.length(),
+                                       value.c_str(), value.length(),
+                                       expiration, flags);
+    return (rc == MEMCACHED_SUCCESS || rc == MEMCACHED_BUFFERED);
+  }
+
+  bool set_all(std::vector<std::string> &keys,
+               std::vector<std::string> &values,
+               time_t expiration,
+               uint32_t flags)
+  {
+    if (keys.size() != values.size())
+    {
+      return false;
+    }
+    bool retval= true;
+    std::vector<std::string>::iterator key_it= keys.begin();
+    std::vector<std::string>::iterator val_it= values.begin();
+    while (key_it != keys.end())
+    {
+      retval= set((*key_it), (*val_it), expiration, flags);
+      if (retval == false)
+      {
+        return retval;
+      }
+      ++key_it;
+      ++val_it;
+    }
+    return retval;
+  }
+
+  bool set_by_key(const std::string &master_key, 
+                  const std::string &key, 
+                  const std::string &value,
+                  time_t expiration,
+                  uint32_t flags)
+  {
+    memcached_return rc= memcached_set_by_key(&memc, master_key.c_str(), 
+                                              master_key.length(),
+                                              key.c_str(), key.length(),
+                                              value.c_str(), value.length(),
+                                              expiration,
+                                              flags);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool increment(const std::string &key, unsigned int offset, uint64_t *value)
+  {
+    memcached_return rc= memcached_increment(&memc, key.c_str(), key.length(),
+                                             offset, value);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool decrement(const std::string &key, unsigned int offset, uint64_t *value)
+  {
+    memcached_return rc= memcached_decrement(&memc, key.c_str(), 
+                                             key.length(),
+                                             offset, value);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+
+  bool add(const std::string &key, const std::string &value)
+  {
+    memcached_return rc= memcached_add(&memc, key.c_str(), key.length(), 
+                                       value.c_str(), value.length(), 0, 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool add_by_key(const std::string &master_key, 
+                  const std::string &key, 
+                  const std::string &value)
+  {
+    memcached_return rc= memcached_add_by_key(&memc, 
+                                              master_key.c_str(),
+                                              master_key.length(),
+                                              key.c_str(),
+                                              key.length(),
+                                              value.c_str(), 
+                                              value.length(),
+                                              0, 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool replace(const std::string &key, const std::string &value)
+  {
+    memcached_return rc= memcached_replace(&memc, key.c_str(), key.length(),
+                                           value.c_str(), value.length(),
+                                           0, 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool replace_by_key(const std::string &master_key, 
+                      const std::string &key, 
+                      const std::string &value)
+  {
+    memcached_return rc= memcached_replace_by_key(&memc, 
+                                                  master_key.c_str(), 
+                                                  master_key.length(),
+                                                  key.c_str(), 
+                                                  key.length(),
+                                                  value.c_str(), 
+                                                  value.length(), 
+                                                  0, 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool prepend(const std::string &key, const std::string &value)
+  {
+    memcached_return rc= memcached_prepend(&memc, key.c_str(), key.length(),
+                                           value.c_str(), value.length(), 0, 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool prepend_by_key(const std::string &master_key, 
+                      const std::string &key, 
+                      const std::string &value)
+  {
+    memcached_return rc= memcached_prepend_by_key(&memc, 
+                                                  master_key.c_str(), 
+                                                  master_key.length(),
+                                                  key.c_str(), 
+                                                  key.length(),
+                                                  value.c_str(), 
+                                                  value.length(),
+                                                  0,
+                                                  0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool append(const std::string &key, const std::string &value)
+  {
+    memcached_return rc= memcached_append(&memc, 
+                                          key.c_str(), 
+                                          key.length(),
+                                          value.c_str(), 
+                                          value.length(), 
+                                          0, 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool append_by_key(const std::string &master_key, 
+                     const std::string &key, 
+                     const std::string &value)
+  {
+    memcached_return rc= memcached_append_by_key(&memc,
+                                                 master_key.c_str(), 
+                                                 master_key.length(),
+                                                 key.c_str(), 
+                                                 key.length(),
+                                                 value.c_str(), 
+                                                 value.length(), 
+                                                 0, 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool cas(const std::string &key, 
+           const std::string &value, 
+           uint64_t cas_arg)
+  {
+    memcached_return rc= memcached_cas(&memc, key.c_str(), key.length(),
+                                       value.c_str(), value.length(), 
+                                       0, 0, cas_arg);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool cas_by_key(const std::string &master_key, 
+                  const std::string &key, 
+                  const std::string &value, 
+                  uint64_t cas_arg)
+  {
+    memcached_return rc= memcached_cas_by_key(&memc,
+                                              master_key.c_str(), 
+                                              master_key.length(),
+                                              key.c_str(), 
+                                              key.length(),
+                                              value.c_str(), 
+                                              value.length(),
+                                              0, 0, cas_arg);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  // using 'remove' vs. 'delete' since 'delete' is a keyword 
+  bool remove(const std::string &key)
+  {
+    memcached_return rc= memcached_delete(&memc, key.c_str(), key.length(), 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool delete_by_key(const std::string &master_key, 
+                     const std::string &key)
+  {
+    memcached_return rc= memcached_delete_by_key(&memc, 
+                                                 master_key.c_str(), 
+                                                 master_key.length(),
+                                                 key.c_str(), 
+                                                 key.length(), 
+                                                 0);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool flush(time_t expiration)
+  {
+    memcached_return rc= memcached_flush(&memc, expiration);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  bool fetch_execute(memcached_execute_function *callback,
+                     void *context,
+                     unsigned int num_of_callbacks)
+  {
+    memcached_return rc= memcached_fetch_execute(&memc,
+                                                 callback,
+                                                 context,
+                                                 num_of_callbacks);
+    return (rc == MEMCACHED_SUCCESS);
+  }
+
+  const std::string lib_version() const
+  {
+    const char *ver= memcached_lib_version();
+    const std::string version(ver);
+    return version;
+  }
+
+private:
+  memcached_st memc;
+  memcached_result_st result;
 };

=== modified file 'tests/plus.cpp'
--- tests/plus.cpp	2009-07-07 21:57:24 +0000
+++ tests/plus.cpp	2009-07-11 19:10:39 +0000
@@ -16,87 +16,184 @@
 
 #include "test.h"
 
+#include <string>
+
+using namespace std;
+
 extern "C" {
-   test_return basic_test(memcached_st *memc);
-   uint8_t increment_test(memcached_st *memc);
-   test_return basic_master_key_test(memcached_st *memc);
    void *world_create(void);
    void world_destroy(void *p);
 }
 
-test_return basic_test(memcached_st *memc)
+static test_return basic_test(memcached_st *memc)
 {
   Memcached foo(memc);
-  const char *value_set= "This is some data";
-  char *value;
+  const string value_set("This is some data");
+  string value;
   size_t value_length;
 
-  foo.set("mine", value_set, strlen(value_set));
+  foo.set("mine", value_set, 0, 0);
   value= foo.get("mine", &value_length);
 
-  assert((memcmp(value, value_set, value_length) == 0));
+  assert((memcmp(value.c_str(), value_set.c_str(), value_length) == 0));
 
   return TEST_SUCCESS;
 }
 
-uint8_t increment_test(memcached_st *memc)
+static test_return increment_test(memcached_st *memc)
 {
   Memcached mcach(memc);
-  memcached_return rc;
-  const char *key= "inctest";
-  const char *inc_value= "1";
-  char *ret_value;
+  bool rc;
+  const string key("inctest");
+  const string inc_value("1");
+  string ret_value;
   uint64_t int_inc_value;
   uint64_t int_ret_value;
   size_t value_length;
 
-  mcach.set(key, inc_value, strlen(inc_value));
+  mcach.set(key, inc_value, 0, 0);
   ret_value= mcach.get(key, &value_length);
-  printf("\nretvalue %s\n",ret_value);
-  int_inc_value= uint64_t(atol(inc_value));
-  int_ret_value= uint64_t(atol(ret_value));
+  printf("\nretvalue %s\n",ret_value.c_str());
+  int_inc_value= uint64_t(atol(inc_value.c_str()));
+  int_ret_value= uint64_t(atol(ret_value.c_str()));
   assert(int_ret_value == int_inc_value); 
 
   rc= mcach.increment(key, 1, &int_ret_value);
-  assert(rc == MEMCACHED_SUCCESS);
+  assert(rc == true);
   assert(int_ret_value == 2);
 
   rc= mcach.increment(key, 1, &int_ret_value);
-  assert(rc == MEMCACHED_SUCCESS);
+  assert(rc == true);
   assert(int_ret_value == 3);
 
   rc= mcach.increment(key, 5, &int_ret_value);
-  assert(rc == MEMCACHED_SUCCESS);
+  assert(rc == true);
   assert(int_ret_value == 8);
 
-  return 0;
+  return TEST_SUCCESS;
 }
 
-test_return basic_master_key_test(memcached_st *memc)
+static test_return basic_master_key_test(memcached_st *memc)
 {
-   Memcached foo(memc);
-  const char *value_set= "Data for server A";
-  const char *master_key_a= "server-a";
-  const char *master_key_b= "server-b";
-  const char *key= "xyz";
-  char *value;
+  Memcached foo(memc);
+  const string value_set("Data for server A");
+  const string master_key_a("server-a");
+  const string master_key_b("server-b");
+  const string key("xyz");
+  string value;
   size_t value_length;
 
-  foo.set_by_key(master_key_a, key, value_set, strlen(value_set));
+  foo.set_by_key(master_key_a, key, value_set, 0, 0);
   value= foo.get_by_key(master_key_a, key, &value_length);
 
-  assert((memcmp(value, value_set, value_length) == 0));
+  assert((memcmp(value.c_str(), value_set.c_str(), value_length) == 0));
 
   value= foo.get_by_key(master_key_b, key, &value_length);
-  assert((memcmp(value, value_set, value_length) == 0));
-
-  return TEST_SUCCESS;
-}
-
+  assert((memcmp(value.c_str(), value_set.c_str(), value_length) == 0));
+
+  return TEST_SUCCESS;
+}
+
+/* Count the results */
+static memcached_return callback_counter(memcached_st *ptr __attribute__((unused)), 
+                                     memcached_result_st *result __attribute__((unused)), 
+                                     void *context)
+{
+  unsigned int *counter= static_cast<unsigned int *>(context);
+
+  *counter= *counter + 1;
+
+  return MEMCACHED_SUCCESS;
+}
+
+static test_return mget_result_function(memcached_st *memc)
+{
+  Memcached mc(memc);
+  bool rc;
+  string key1("fudge");
+  string key2("son");
+  string key3("food");
+  vector<string> keys;
+  keys.reserve(3);
+  keys.push_back(key1);
+  keys.push_back(key2);
+  keys.push_back(key3);
+  unsigned int counter;
+  memcached_execute_function callbacks[1];
+
+  /* We need to empty the server before we continue the test */
+  rc= mc.flush(0);
+  rc= mc.set_all(keys, keys, 50, 9);
+  assert(rc == true);
+
+  rc= mc.mget(keys);
+  assert(rc == true);
+
+  callbacks[0]= &callback_counter;
+  counter= 0;
+  rc= mc.fetch_execute(callbacks, static_cast<void *>(&counter), 1); 
+
+  assert(counter == 3);
+
+  return TEST_SUCCESS;
+}
+
+static test_return mget_test(memcached_st *memc)
+{
+  Memcached mc(memc);
+  bool rc;
+  memcached_return mc_rc;
+  vector<string> keys;
+  keys.reserve(3);
+  keys.push_back("fudge");
+  keys.push_back("son");
+  keys.push_back("food");
+  uint32_t flags;
+
+  string return_key;
+  size_t return_key_length;
+  string return_value;
+  size_t return_value_length;
+
+  /* We need to empty the server before we continue the test */
+  rc= mc.flush(0);
+  assert(rc == true);
+
+  rc= mc.mget(keys);
+  assert(rc == true);
+
+  while (mc.fetch(return_key, return_value, &return_key_length, 
+                  &return_value_length, &flags, &mc_rc))
+  {
+    assert(return_value.length() != 0);
+  }
+  assert(return_value_length == 0);
+  assert(mc_rc == MEMCACHED_END);
+
+  rc= mc.set_all(keys, keys, 50, 9);
+  assert(rc == true);
+
+  rc= mc.mget(keys);
+  assert(rc == true);
+
+  while ((mc.fetch(return_key, return_value, &return_key_length, 
+                   &return_value_length, &flags, &mc_rc)))
+  {
+    assert(return_value.length() != 0);
+    assert(mc_rc == MEMCACHED_SUCCESS);
+    assert(return_key_length == return_value_length);
+    assert(!memcmp(return_value.c_str(), return_key.c_str(), return_value_length));
+  }
+
+  return TEST_SUCCESS;
+}
 
 test_st tests[] ={
-  {"basic", 0, basic_test },
-  {"basic_master_key", 0, basic_master_key_test },
+  { "basic", 0, basic_test },
+  { "basic_master_key", 0, basic_master_key_test },
+  { "increment_test", 0, increment_test },
+  { "mget", 1, mget_test },
+  { "mget_result_function", 1, mget_result_function },
   {0, 0, 0}
 };