Feature request: conditional shdict:set #180

Open
Thomas12 opened this Issue Dec 2, 2012 · 15 comments

Comments

Projects
None yet
5 participants

Thomas12 commented Dec 2, 2012

Hi,

could you please implement this function:

ret=shdict:set_conditional(key, new_value, old_value)

Interpretation: only set "key" to "new_value" if currently "key" contains "old_value". If "new_value" is set return true, otherwise false.

Without such a function, re-caching leads to many parallel re-cache attempts as the "expired-condition" is true for too many concurrent requests resulting in load-peaks. To only allow ONE update one needs such a conditional set.

Thank you!

Thomas

Member

bakins commented Dec 2, 2012

So, "compare and swap"? Would you want to be able to update expires and/or flags if value is changed?

Thomas12 commented Dec 2, 2012

Hi, yes, it is the "compare and swap" idea. Updating the expiretime and flags would also be a nice addon.

Member

bakins commented Dec 2, 2012

On Dec 2, 2012, at 6:24 PM, Thomas12 notifications@github.com wrote:

Hi, yes, it is the "compare and swap" idea. Updating the expiretime and flags would also be a nice addon.

Looking at the code, it wouldn't be too hard. @agentzh, I was thinking it could be another option to ngx_http_lua_shdict_set_helper, however that function is already rather lengthy and a compare-and-swap would need different arguments (well, an additional one). This is why I think it'd be good to split out shdict ;)

I was thinking a function like:

success, err, forcible = ngx.shared.DICT.cas((key, old_value, new_value, exptime?, flags?)

but this would require manipulating the set_helper argument parsing a bit. I can give it a stab if you think that's fine. Admittedly, this would be much easier if the shdict was written mostly in Lua with just some minor C bindings - preferably using FFI. Maybe if we exposed more as library calls?

Owner

agentzh commented Dec 4, 2012

Hello!

On Sun, Dec 2, 2012 at 3:44 PM, Brian Akins notifications@github.comwrote:

I was thinking a function like:

success, err, forcible = ngx.shared.DICT.cas((key, old_value, new_value,
exptime?, flags?)

This looks good to me :)

but this would require manipulating the set_helper argument parsing a bit.
I can give it a stab if you think that's fine. Admittedly, this would be
much easier if the shdict was written mostly in Lua with just some minor C
bindings - preferably using FFI. Maybe if we exposed more as library calls?

For now, FFI-based API only helps if the user Lua code is actually JIT
compiled (which can be checked by the jit.v or jit.dump libraries shipped
with LuaJIT 2.0).

The FFI-based APIs will lead to significant performance loss if the user
code is interpreted by LuaJIT 2.0 however. See the discussion here:

http://www.freelists.org/post/luajit/Speeding-up-FFI-in-the-LuaJIT-interpreter

Best regards,
-agentzh

simpl commented Dec 4, 2012

It might be useful to some to add the memcached-like methods of append and prepend as well.

The FFI-based APIs will lead to significant performance loss if the user code is interpreted by LuaJIT 2.0 however.

Are there plans to impelement FFI-based libraries for things like JSON parsing (perhaps wrapping around the core functions of e.g. cjson) to prevent the JIT compiler dropping into interpreter mode?

When FFI in interpreter-mode is so slow, is there a way to force luajit to jit-compile everything?

Owner

agentzh commented Dec 10, 2012

Hello!

On Mon, Dec 10, 2012 at 3:02 PM, Thomas12 notifications@github.com wrote:

When FFI in interpreter-mode is so slow, is there a way to force luajit to
jit-compile everything?

I don't think so. Also, there's quite a few primitives are not yet
implemented in the JIT compiler of LuaJIT 2.0:

http://wiki.luajit.org/NYI

So we should really first get all these NYI items supported in the JIT
compiler first and then look into a way to force JIT compiling.

Best regards,
-agentzh

Couldn't one just use "ngx_http_lua_shdict_incr" as basis and change:

  1. (n!=4) //4 arguments
  2. instead of just value as int, get oldval and newval as luaL_checklstring(L, 3..) and (L,4..)
  3. instead of checking for "not a number" just compare current fetched value with oldval and if it fails, just return as "not a number" does but with changed error-string.
  4. When 3) succeeds instead, overwrite "key" with newval instead of just incrementing it.

(I can read source but not modify it as I don't fully understand some details like the p-pointer).

Member

bakins commented Dec 15, 2012

I haven't had a chance to start coding yet, so I can look at using the incr code as a starting place.

Thomas12 commented Feb 3, 2013

Any news on compare_and_swap?

I think it is even better to put cas before the "replace" paragraph of the ngx_http_lua_shdict_set_helper.
There one just had to check the testvalue with the actual value in the cache.
If match -> goto replace:, if not, jump out before and do nothing.

Thomas12 commented Feb 3, 2013

Here's a cas-patch, not sure if it is 100% correct, testscript seems to work.
Remaining issue: This only works for strings right now, need to implement lua_type, too, but what to do? Has a "100" stored as number to be considered the same as a "100" stored as string when comparing?

Updated patch: now supports strings, numbers, ...
Open: How to behave when a string "100" is stored and later compared with a number 100? Strict or weak types?

--- ngx_http_lua_shdict.c       2012-12-14 22:46:25.000000000 +0100
+++ ngx_http_lua_shdict.c       2013-02-03 18:32:59.000000000 +0100
@@ -745,6 +745,7 @@
     int                          i, n;
     ngx_str_t                    name;
     ngx_str_t                    key;
+    ngx_str_t                    compare;
     uint32_t                     hash;
     ngx_int_t                    rc;
     ngx_http_lua_shdict_ctx_t   *ctx;
@@ -753,6 +754,8 @@
     int                          value_type;
     lua_Number                   num;
     u_char                       c;
+    lua_Number                   num2;
+    u_char                       c2;
     lua_Number                   exptime = 0;
     u_char                      *p;
     ngx_rbtree_node_t           *node;
@@ -765,8 +768,8 @@

     n = lua_gettop(L);

-    if (n != 3 && n != 4 && n != 5) {
-        return luaL_error(L, "expecting 3, 4 or 5 arguments, "
+    if (n != 3 && n != 4 && n != 5 && n != 6) {
+        return luaL_error(L, "expecting 3, 4, 5 or 6 arguments, "
                 "but only seen %d", n);
     }

@@ -836,7 +839,7 @@
         }
     }

-    if (n == 5) {
+    if (n >= 5) {
         user_flags = (uint32_t) luaL_checkinteger(L, 5);
     }

@@ -898,6 +901,49 @@
             goto remove;
         }

+    if (n == 6) {
+        value_type = lua_type(L, 6);
+
+        switch (value_type) {
+        case LUA_TSTRING:
+            compare.data = (u_char *) lua_tolstring(L, 6, &compare.len);
+            break;
+
+        case LUA_TNUMBER:
+            compare.len = sizeof(lua_Number);
+            num2 = lua_tonumber(L, 6);
+            compare.data = (u_char *) &num2;
+            break;
+
+        case LUA_TBOOLEAN:
+            compare.len = sizeof(u_char);
+            c2 = lua_toboolean(L, 6) ? 1 : 0;
+            compare.data = &c2;
+            break;
+
+        case LUA_TNIL:
+            compare.len = 0;
+            compare.data = NULL;
+            break;
+
+        default:
+            ngx_shmtx_unlock(&ctx->shpool->mutex);
+            return luaL_error(L, "unsupported value type for key \"%s\" in "
+                "shared_dict \"%s\": %s", key.data, name.data,
+                lua_typename(L, value_type));
+        }
+
+        //memcmp used as comparing 2 integers leads to null-char vs. null-char -> strncmp stops and falsely reports: identical
+        if( (compare.len != sd->value_len) || (ngx_memcmp(compare.data, sd->data+sd->key_len, sd->value_len) != 0) ) {
+             ngx_shmtx_unlock(&ctx->shpool->mutex);
+
+             lua_pushboolean(L, 0);
+             lua_pushliteral(L, "cas: compare!=value");
+             lua_pushboolean(L, forcible);
+             return 3;
+          }
+    }
+
 replace:
         if (value.data && value.len == (size_t) sd->value_len) {


and here a test-script:

local shmdat=ngx.shared.shmdat;

shmdat:set("test", "100", 0, 0);
local g=shmdat:get("test") or 0;
ngx.print("1. g="..g.."<br>")

ret, err = shmdat:set("test", "2000", 0, 0, "1500");
if(ret==false) then ret=0; else ret=1; end;
if(err==nil) then err="no"; end;
local g=shmdat:get("test") or 0;
ngx.print("2. failed cas="..g..", ret="..ret..", err="..err.."<br>")

ret, err = shmdat:set("test", "2000", 0, 0, "100");
if(ret==false) then ret=0; else ret=1; end;
if(err==nil) then err="no"; end;
local g=shmdat:get("test") or 0;
ngx.print("3. successful cas="..g..", ret="..ret..", err="..err.."<br>")

ngx.print("<hr>")

shmdat:set("test2", 100, 0, 0);
local g=shmdat:get("test2") or 0;
ngx.print("1. g="..g.."<br>")

ret, err = shmdat:set("test2", 2000, 0, 0, 1500);
if(ret==false) then ret=0; else ret=1; end;
if(err==nil) then err="no"; end;
local g=shmdat:get("test2") or 0;
ngx.print("2. failed cas="..g..", ret="..ret..", err="..err.."<br>")

ret, err = shmdat:set("test2", 2000, 0, 0, 100);
if(ret==false) then ret=0; else ret=1; end;
if(err==nil) then err="no"; end;
local g=shmdat:get("test2") or 0;
ngx.print("3. successful cas="..g..", ret="..ret..", err="..err.."<br>")

Owner

agentzh commented Feb 4, 2013

Hello!

On Sun, Feb 3, 2013 at 7:38 AM, Thomas12 notifications@github.com wrote:

Here's a cas-patch, not sure if it is 100% correct, testscrpt seems to
work :

Thank you for your patch! I'll look into this when I have some more time :)

Best regards,
-agentzh

Contributor

mtourne commented Feb 5, 2013

"Conditional set" seems interesting, to avoid incurring the cost of "getting the data" if it does exist in the set.
However, I think a new method has_key() is more generic and lets you do a "conditional set" just as easily in the Lua user code:

local key = "foo"
if not my_shdict:has_key(key) then
     my_shdict:set(key, "bar")
end
Member

bakins commented Feb 5, 2013

@mtourne in your case, those 2 operations aren't atomic together, the cas would be.

I just haven't had the time to hack on this.

Owner

agentzh commented Feb 16, 2016

This feature is now implemented as the cas method in @doujiang24's this PR:

#583

Feedback welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment