tarantool/avro-schema

Consider replacing back fstack with debug.getlocal()

Totktonada opened this issue · 1 comments

It may give some performance on success paths.

We replaced debug.getlocal() approach with fstack in order to fix #11. Later, @mejedi investigated the problem further and proposed a work around.

Workaround.
From 322196d5729b83b114af48e243487f1fe835ec56 Mon Sep 17 00:00:00 2001
From: Nick Zavaritsky <mejedi@gmail.com>
Date: Fri, 28 Sep 2018 16:25:42 +0000
Subject: [PATCH] Bugfix

---
 avro_schema/frontend.lua | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/avro_schema/frontend.lua b/avro_schema/frontend.lua
index 10f8ae4..e9dabbd 100644
--- a/avro_schema/frontend.lua
+++ b/avro_schema/frontend.lua
@@ -568,17 +568,14 @@ copy_data = function(schema, data, visited)
         if data ~= null then
             error()
         end
-        return null
     elseif schematype == 'boolean' then
         if type(data) ~= 'boolean' then
             error()
         end
-        return data
     elseif schematype == 'int' then
         if data < -2147483648 or data > 2147483647 or floor(tonumber(data)) ~= data then
             error()
         end
-        return data
     elseif schematype == 'long' then
         local n = tonumber(data)
         -- note: if it's not a number or cdata(numbertype),
@@ -596,24 +593,20 @@ copy_data = function(schema, data, visited)
                 error()
             end
         end
-        return data
     elseif schematype == 'double' or schema == 'float' then
-        return 0 + tonumber(data)
+        data = 0 + tonumber(data)
     elseif schematype == 'bytes' or schematype == 'string' then
         if type(data) ~= 'string' then
             error()
         end
-        return data
     elseif schematype == 'enum' then
         if not get_enum_symbol_map(schema)[data] then
             error()
         end
-        return data
     elseif schematype == 'fixed' then
         if type(data) ~= 'string' or #data ~= schema.size then
             error()
         end
-        return data
     else
         if visited[data] then
             error('@Infinite loop detected in the data', 0)
@@ -689,8 +682,11 @@ copy_data = function(schema, data, visited)
             assert(false)
         end
         visited[data] = nil
-        return res
+        data = res
     end
+    goto l
+::l::
+    return data
 end

 -- extract from the call stack a path to the fragment that failed
--
2.17.1

Let's compare performance and decide.

@Totktonada

There is a test branch for this purpose - https://github.com/tarantool/avro-schema/tree/139-fstack-replace

I've run "benchmark.lua" before and after this commit. My results:

  • Before: 0.247 M RPS
  • After: 0.270 M RPS

Seems it looks reasonable to use debug.getlocal() instead of fstack.