Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] sysprof: disable proto and trace dumps in default
@ 2022-06-12 18:52 Maxim Kokryashkin via Tarantool-patches
  2022-06-16  6:56 ` Sergey Kaplun via Tarantool-patches
  2022-07-15 13:36 ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 4+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-06-12 18:52 UTC (permalink / raw)
  To: tarantool-patches, imun, skaplun

There is no need to dump proto or trace information for sysprof
in the default mode. Moreover, attempts to do so will lead to
segmentation fault due to uninitialized buffer. This commit
disables proto and trace dumps in the default mode.

Resolves tarantool/tarantool#7264
---
PR: https://github.com/tarantool/tarantool/pull/7265
Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-7264-dump-proto-default-mode

 src/lj_sysprof.c                              |  4 +-
 ...4-add-proto-trace-sysprof-default.test.lua | 58 +++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua

diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index a4a70146..2e9ed9b3 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -524,7 +524,7 @@ void lj_sysprof_add_proto(const struct GCproto *pt)
 {
   struct sysprof *sp = &sysprof;
 
-  if (sp->state != SPS_PROFILE)
+  if (sp->state != SPS_PROFILE || sp->opt.mode == LUAM_SYSPROF_DEFAULT)
     return;
 
   /*
@@ -543,7 +543,7 @@ void lj_sysprof_add_trace(const struct GCtrace *tr)
 {
   struct sysprof *sp = &sysprof;
 
-  if (sp->state != SPS_PROFILE)
+  if (sp->state != SPS_PROFILE || sp->opt.mode == LUAM_SYSPROF_DEFAULT)
     return;
 
   /* See the comment about the sysprof state above. */
diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
new file mode 100644
index 00000000..2d2a756a
--- /dev/null
+++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
@@ -0,0 +1,58 @@
+-- Sysprof is implemented for x86 and x64 architectures only.
+local ffi = require("ffi")
+require("utils").skipcond(
+  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
+    or ffi.abi("gc64"),
+  jit.arch.." architecture or "..jit.os..
+  " OS is NIY for sysprof"
+)
+
+local tap = require("tap")
+local jit = require("jit")
+
+local test = tap.test("gh-7264-add-proto-trace-sysprof-default.test.lua")
+test:plan(2)
+
+jit.off()
+jit.flush()
+
+local function allocate()
+  local a = {}
+  for _ = 1, 1e4 do table.insert(a, "teststring") end
+  return a
+end
+
+local chunk = [[
+function lua_global_f()
+  local a = 'teststring'
+end
+]]
+
+-- Trace creation during the sysprof runtime.
+jit.on()
+
+local _ = nil
+local res, err = misc.sysprof.start({ mode = "D" })
+assert(res, err)
+allocate()
+res, _ = misc.sysprof.stop()
+test:ok(res)
+
+jit.off()
+
+-- Proto creation during the sysprof runtime.
+res, err = misc.sysprof.start({ mode = "D" })
+assert(res, err)
+
+res, err = loadstring(chunk)
+assert(res, err)
+res()
+
+-- luacheck: globals lua_global_f
+lua_global_f()
+collectgarbage()
+res, _ = misc.sysprof.stop()
+test:ok(res)
+
+jit.on()
+os.exit(test:check() and 0 or 1)
-- 
2.36.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] sysprof: disable proto and trace dumps in default
  2022-06-12 18:52 [Tarantool-patches] [PATCH luajit] sysprof: disable proto and trace dumps in default Maxim Kokryashkin via Tarantool-patches
@ 2022-06-16  6:56 ` Sergey Kaplun via Tarantool-patches
  2022-07-04 15:27   ` Maxim Kokryashkin via Tarantool-patches
  2022-07-15 13:36 ` Igor Munkin via Tarantool-patches
  1 sibling, 1 reply; 4+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-06-16  6:56 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Maxim!

Thanks for the patch!
LGTM, except a few nits below.

On 12.06.22, Maxim Kokryashkin wrote:
> There is no need to dump proto or trace information for sysprof
> in the default mode. Moreover, attempts to do so will lead to
> segmentation fault due to uninitialized buffer. This commit
> disables proto and trace dumps in the default mode.
> 
> Resolves tarantool/tarantool#7264
> ---
> PR: https://github.com/tarantool/tarantool/pull/7265
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-7264-dump-proto-default-mode
> 
>  src/lj_sysprof.c                              |  4 +-
>  ...4-add-proto-trace-sysprof-default.test.lua | 58 +++++++++++++++++++
>  2 files changed, 60 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> 
> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> index a4a70146..2e9ed9b3 100644
> --- a/src/lj_sysprof.c
> +++ b/src/lj_sysprof.c

<snipped>

> diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> new file mode 100644
> index 00000000..2d2a756a
> --- /dev/null
> +++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> @@ -0,0 +1,58 @@
> +-- Sysprof is implemented for x86 and x64 architectures only.
> +local ffi = require("ffi")
> +require("utils").skipcond(
> +  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
> +    or ffi.abi("gc64"),
> +  jit.arch.." architecture or "..jit.os..
> +  " OS is NIY for sysprof"
> +)
> +
> +local tap = require("tap")
> +local jit = require("jit")
> +
> +local test = tap.test("gh-7264-add-proto-trace-sysprof-default.test.lua")
> +test:plan(2)
> +
> +jit.off()
> +jit.flush()
> +
> +local function allocate()
> +  local a = {}
> +  for _ = 1, 1e4 do table.insert(a, "teststring") end

Why do we need so many iterations?
We can manipulate JIT behaviour by using `jit.opt.start('hotloop=1')`.

> +  return a
> +end
> +
> +local chunk = [[
> +function lua_global_f()
> +  local a = 'teststring'

Nit: I suggest to use one type of quotes in the whole test.
(Also, it is better to use single quotes everywhere (its offline
agreement with Igor).

> +end
> +]]
> +
> +-- Trace creation during the sysprof runtime.
> +jit.on()
> +
> +local _ = nil

Why do we need this variable? (If for the `res, _ =` below, we can just
use the single `res = `).

> +local res, err = misc.sysprof.start({ mode = "D" })
> +assert(res, err)
> +allocate()
> +res, _ = misc.sysprof.stop()
> +test:ok(res)

Nit: please add name for the test.

> +
> +jit.off()
> +
> +-- Proto creation during the sysprof runtime.
> +res, err = misc.sysprof.start({ mode = "D" })
> +assert(res, err)
> +
> +res, err = loadstring(chunk)
> +assert(res, err)
> +res()
> +
> +-- luacheck: globals lua_global_f
> +lua_global_f()
> +collectgarbage()
> +res, _ = misc.sysprof.stop()
> +test:ok(res)

Ditto.

> +
> +jit.on()
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.36.1
> 

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit] sysprof: disable proto and trace dumps in default
  2022-06-16  6:56 ` Sergey Kaplun via Tarantool-patches
@ 2022-07-04 15:27   ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 0 replies; 4+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-07-04 15:27 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: Maxim Kokryashkin, tarantool-patches

[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]


Hi, Sergey!
Thanks for the review!
I’ve fixed everything you requested and updated the corresponding branch.
--
Best regards,
Maxim Kokryashkin
 
  
>Четверг, 16 июня 2022, 9:58 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>
>Thanks for the patch!
>LGTM, except a few nits below.
>
>On 12.06.22, Maxim Kokryashkin wrote:
>> There is no need to dump proto or trace information for sysprof
>> in the default mode. Moreover, attempts to do so will lead to
>> segmentation fault due to uninitialized buffer. This commit
>> disables proto and trace dumps in the default mode.
>>
>> Resolves tarantool/tarantool#7264
>> ---
>> PR:  https://github.com/tarantool/tarantool/pull/7265
>> Branch:  https://github.com/tarantool/luajit/tree/fckxorg/gh-7264-dump-proto-default-mode
>>
>> src/lj_sysprof.c | 4 +-
>> ...4-add-proto-trace-sysprof-default.test.lua | 58 +++++++++++++++++++
>> 2 files changed, 60 insertions(+), 2 deletions(-)
>> create mode 100644 test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
>>
>> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
>> index a4a70146..2e9ed9b3 100644
>> --- a/src/lj_sysprof.c
>> +++ b/src/lj_sysprof.c
>
><snipped>
>
>> diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
>> new file mode 100644
>> index 00000000..2d2a756a
>> --- /dev/null
>> +++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
>> @@ -0,0 +1,58 @@
>> +-- Sysprof is implemented for x86 and x64 architectures only.
>> +local ffi = require("ffi")
>> +require("utils").skipcond(
>> + jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
>> + or ffi.abi("gc64"),
>> + jit.arch.." architecture or "..jit.os..
>> + " OS is NIY for sysprof"
>> +)
>> +
>> +local tap = require("tap")
>> +local jit = require("jit")
>> +
>> +local test = tap.test("gh-7264-add-proto-trace-sysprof-default.test.lua")
>> +test:plan(2)
>> +
>> +jit.off()
>> +jit.flush()
>> +
>> +local function allocate()
>> + local a = {}
>> + for _ = 1, 1e4 do table.insert(a, "teststring") end
>
>Why do we need so many iterations?
>We can manipulate JIT behaviour by using `jit.opt.start('hotloop=1')`.
>
>> + return a
>> +end
>> +
>> +local chunk = [[
>> +function lua_global_f()
>> + local a = 'teststring'
>
>Nit: I suggest to use one type of quotes in the whole test.
>(Also, it is better to use single quotes everywhere (its offline
>agreement with Igor).
>
>> +end
>> +]]
>> +
>> +-- Trace creation during the sysprof runtime.
>> +jit.on()
>> +
>> +local _ = nil
>
>Why do we need this variable? (If for the `res, _ =` below, we can just
>use the single `res = `).
>
>> +local res, err = misc.sysprof.start({ mode = "D" })
>> +assert(res, err)
>> +allocate()
>> +res, _ = misc.sysprof.stop()
>> +test:ok(res)
>
>Nit: please add name for the test.
>
>> +
>> +jit.off()
>> +
>> +-- Proto creation during the sysprof runtime.
>> +res, err = misc.sysprof.start({ mode = "D" })
>> +assert(res, err)
>> +
>> +res, err = loadstring(chunk)
>> +assert(res, err)
>> +res()
>> +
>> +-- luacheck: globals lua_global_f
>> +lua_global_f()
>> +collectgarbage()
>> +res, _ = misc.sysprof.stop()
>> +test:ok(res)
>
>Ditto.
>
>> +
>> +jit.on()
>> +os.exit(test:check() and 0 or 1)
>> --
>> 2.36.1
>>
>
>--
>Best regards,
>Sergey Kaplun
 

[-- Attachment #2: Type: text/html, Size: 4523 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] sysprof: disable proto and trace dumps in default
  2022-06-12 18:52 [Tarantool-patches] [PATCH luajit] sysprof: disable proto and trace dumps in default Maxim Kokryashkin via Tarantool-patches
  2022-06-16  6:56 ` Sergey Kaplun via Tarantool-patches
@ 2022-07-15 13:36 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 4+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-07-15 13:36 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Max,

Thanks for the patch! LGTM, with some nits I've fixed by myself. You can
find the resulting patch here[1].

[1]: https://github.com/tarantool/luajit/commit/02d655c

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-07-15 13:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-12 18:52 [Tarantool-patches] [PATCH luajit] sysprof: disable proto and trace dumps in default Maxim Kokryashkin via Tarantool-patches
2022-06-16  6:56 ` Sergey Kaplun via Tarantool-patches
2022-07-04 15:27   ` Maxim Kokryashkin via Tarantool-patches
2022-07-15 13:36 ` Igor Munkin via Tarantool-patches

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