Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, kyukhin@tarantool.org
Subject: [Tarantool-patches] [PATCH 08/15] uuid: replace static_alloc with ffi stash
Date: Wed, 24 Mar 2021 22:24:34 +0100	[thread overview]
Message-ID: <45068565dd18f7e65f534930d2ce6208b0ef2f43.1616620860.git.v.shpilevoy@tarantool.org> (raw)
In-Reply-To: <cover.1616620860.git.v.shpilevoy@tarantool.org>

static_alloc() appears not to be safe to use in Lua, because it
does not provide any ownership protection for the returned values.

The problem appears when something is allocated, then Lua GC
starts, and some __gc handlers might also use static_alloc(). In
Lua and in C - both lead to the buffer being corrupted in its
original usage place.

The patch is a part of activity of getting rid of static_alloc()
in Lua. It removes it from uuid Lua module and makes it use the
new FFI stash feature, which helps to cache frequently used and
heavy to allocate FFI values.

ffi.new() is not used, because

- It produces a new GC object;

- ffi.new('struct tt_uuid') costs around 300ns while FFI stash
  costs around 0.8ns (although it is magically fixed when
  ffi.new('struct tt_uuid[1]') is used);

- Without JIT ffi.new() costs about the same as the stash, ~280ns
  for small objects like tt_uuid.

The patch makes uuid perf a bit better in the places where
static_alloc() was used, because its cost was around 7ns for one
allocation.

(cherry picked from commit 5cbb69077ff9fe22224f94aeaa05d8da9ed9cb6e)
---
 src/lua/uuid.lua     | 24 +++++++++++++++++-------
 test/app/uuid.result |  2 +-
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
index 956ad6e36..46b35075a 100644
--- a/src/lua/uuid.lua
+++ b/src/lua/uuid.lua
@@ -1,6 +1,7 @@
 -- uuid.lua (internal file)
 
 local ffi = require("ffi")
+local buffer = require('buffer')
 local builtin = ffi.C
 
 ffi.cdef[[
@@ -33,7 +34,9 @@ extern const struct tt_uuid uuid_nil;
 local uuid_t = ffi.typeof('struct tt_uuid')
 local UUID_STR_LEN = 36
 local UUID_LEN = ffi.sizeof(uuid_t)
-local uuidbuf = ffi.new(uuid_t)
+local uuid_stash = buffer.ffi_stash_new(uuid_t)
+local uuid_stash_take = uuid_stash.take
+local uuid_stash_put = uuid_stash.put
 
 local uuid_tostring = function(uu)
     if not ffi.istype(uuid_t, uu) then
@@ -69,11 +72,12 @@ local uuid_tobin = function(uu, byteorder)
         return error('Usage: uuid:bin([byteorder])')
     end
     if need_bswap(byteorder) then
-        if uu ~= uuidbuf then
-            ffi.copy(uuidbuf, uu, UUID_LEN)
-        end
+        local uuidbuf = uuid_stash_take()
+        ffi.copy(uuidbuf, uu, UUID_LEN)
         builtin.tt_uuid_bswap(uuidbuf)
-        return ffi.string(ffi.cast('char *', uuidbuf), UUID_LEN)
+        uu = ffi.string(ffi.cast('char *', uuidbuf), UUID_LEN)
+        uuid_stash_put(uuidbuf)
+        return uu
     end
     return ffi.string(ffi.cast('char *', uu), UUID_LEN)
 end
@@ -114,12 +118,18 @@ local uuid_new = function()
 end
 
 local uuid_new_bin = function(byteorder)
+    local uuidbuf = uuid_stash_take()
     builtin.tt_uuid_create(uuidbuf)
-    return uuid_tobin(uuidbuf, byteorder)
+    local res = uuid_tobin(uuidbuf, byteorder)
+    uuid_stash_put(uuidbuf)
+    return res
 end
 local uuid_new_str = function()
+    local uuidbuf = uuid_stash_take()
     builtin.tt_uuid_create(uuidbuf)
-    return uuid_tostring(uuidbuf)
+    local res = uuid_tostring(uuidbuf)
+    uuid_stash_put(uuidbuf)
+    return res
 end
 
 local uuid_mt = {
diff --git a/test/app/uuid.result b/test/app/uuid.result
index 0713614c6..1da8b3e58 100644
--- a/test/app/uuid.result
+++ b/test/app/uuid.result
@@ -106,7 +106,7 @@ uu.node[5]
 -- invalid values
 uuid.fromstr(nil)
 ---
-- error: 'builtin/uuid.lua:47: fromstr(str)'
+- error: 'builtin/uuid.lua:50: fromstr(str)'
 ...
 uuid.fromstr('')
 ---
-- 
2.24.3 (Apple Git-128)


  parent reply	other threads:[~2021-03-24 21:31 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-24 21:24 [Tarantool-patches] [PATCH 00/15] Cord buffer, static alloc, and Lua GC bug for 1.10 Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 01/15] fio: don't use shared buffer in pread() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 10/15] uri: replace static_alloc with ffi stash and ibuf Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 11/15] lua: use lua_pushfstring() instead of tt_sprintf() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 12/15] sio: rework sio_strfaddr() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 13/15] sio: increase SERVICE_NAME_MAXLEN size Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 14/15] sio: introduce and use sio_snprintf() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 15/15] buffer: remove Lua registers Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 02/15] test: don't use IBUF_SHARED in the tests Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 03/15] tuple: pass global ibuf explicitly where possible Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 04/15] iconv: take errno before reseting the context Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 05/15] cord_buf: introduce cord_buf API Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 06/15] cord_buf: introduce ownership management Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 07/15] buffer: implement ffi stash Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 09/15] uuid: drop tt_uuid_str() from Lua Vladislav Shpilevoy via Tarantool-patches
2021-03-29 15:41 ` [Tarantool-patches] [PATCH 00/15] Cord buffer, static alloc, and Lua GC bug for 1.10 Kirill Yukhin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45068565dd18f7e65f534930d2ce6208b0ef2f43.1616620860.git.v.shpilevoy@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=kyukhin@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 08/15] uuid: replace static_alloc with ffi stash' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox