Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal
@ 2021-07-29  9:34 Mikhail Shishatskiy via Tarantool-patches
  2021-08-14 11:24 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 4+ messages in thread
From: Mikhail Shishatskiy via Tarantool-patches @ 2021-07-29  9:34 UTC (permalink / raw)
  To: tarantool-patches, imun, skaplun

There are cases when the memory profiler attempts to attribute
allocations triggered by the JIT engine recording phase
with a Lua function to be recorded. In this case,
lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).

Previously, these situations were ignored and the profiler
reported, that the source line was equal to zero.

This patch adjusts profiler behavior to treat allocations
described above as internal by dumping ASOURCE_INT if
lj_debug_frameline() returns a negative value.

Resolves tarantool/tarantool#5679
---

Issue: https://github.com/tarantool/tarantool/issues/5679
Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal

Changes in v2:
  - Fixed commit title to fit in width of 50 symbols;
  - Rebased to the branch [1];
  - Reused default_payload() as a payload to test patch behavior.

[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number

 src/lj_memprof.c                              | 28 ++++++++++++-------
 .../misclib-memprof-lapi.test.lua             | 16 ++++++++---
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/src/lj_memprof.c b/src/lj_memprof.c
index 87512c3a..e4d819a0 100644
--- a/src/lj_memprof.c
+++ b/src/lj_memprof.c
@@ -108,19 +108,27 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent,
 				GCfunc *fn, struct lua_State *L,
 				cTValue *nextframe)
 {
-  const BCLine line = lj_debug_frameline(L, fn, nextframe);
-  lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC);
-  lj_wbuf_addu64(out, (uintptr_t)funcproto(fn));
   /*
-  ** Line is >= 0 if we are inside a Lua function.
-  ** There are cases when the memory profiler attempts
-  ** to attribute allocations triggered by JIT engine recording
-  ** phase with a Lua function to be recorded. At this case
-  ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
-  ** Equals to zero when LuaJIT is built with the
+  ** Line equals to zero when LuaJIT is built with the
   ** -DLUAJIT_DISABLE_DEBUGINFO flag.
   */
-  lj_wbuf_addu64(out, line >= 0 ? (uint64_t)line : 0);
+  const BCLine line = lj_debug_frameline(L, fn, nextframe);
+
+  if (line < 0) {
+    /*
+    ** Line is >= 0 if we are inside a Lua function.
+    ** There are cases when the memory profiler attempts
+    ** to attribute allocations triggered by JIT engine recording
+    ** phase with a Lua function to be recorded. It this case,
+    ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
+    ** We report such allocations as internal in order not to confuse users.
+    */
+    lj_wbuf_addbyte(out, aevent | ASOURCE_INT);
+  } else {
+    lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC);
+    lj_wbuf_addu64(out, (uintptr_t)funcproto(fn));
+    lj_wbuf_addu64(out, (uint64_t)line);
+  }
 }
 
 static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t aevent,
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index b7e456e1..96864c6a 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
 local tap = require("tap")
 
 local test = tap.test("misc-memprof-lapi")
-test:plan(16)
+test:plan(17)
 
 jit.off()
 jit.flush()
@@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
   for _, event in pairs(events[event_type]) do
     local addr = event.loc.addr
     local traceno = event.loc.traceno
-    if traceno ~= 0 then
+    if traceno ~= 0 and symbols.trace[traceno] then
       local trace_loc = symbols.trace[traceno]
       addr = trace_loc.addr
       ev_type[trace_loc.line] = {
@@ -214,9 +214,17 @@ misc.memprof.stop()
 -- Test profiler with enabled JIT.
 jit.on()
 
--- Pregenerate traces to get expected output:
-default_payload()
+-- On this run traces are generated, JIT-related allocations
+-- will be recorded as well.
+symbols, events = generate_parsed_output(default_payload)
+
+alloc = fill_ev_type(events, symbols, "alloc")
+
+-- Test for marking JIT-related allocations as internal.
+-- See also https://github.com/tarantool/tarantool/issues/5679.
+test:ok(alloc[0] == nil)
 
+-- Run already generated traces.
 symbols, events = generate_parsed_output(default_payload)
 
 alloc = fill_ev_type(events, symbols, "alloc")
-- 
2.32.0


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

* Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal
  2021-07-29  9:34 [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal Mikhail Shishatskiy via Tarantool-patches
@ 2021-08-14 11:24 ` Sergey Kaplun via Tarantool-patches
  2021-08-21  4:43   ` Mikhail Shishatskiy via Tarantool-patches
  0 siblings, 1 reply; 4+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-08-14 11:24 UTC (permalink / raw)
  To: Mikhail Shishatskiy; +Cc: tarantool-patches

Hi, thanks for the fixes!

Please, move test adjustment mentioned below to the patch for the symtab
with group allocations by trace's number.
Otherwise, LGTM.

On 29.07.21, Mikhail Shishatskiy wrote:
> There are cases when the memory profiler attempts to attribute
> allocations triggered by the JIT engine recording phase
> with a Lua function to be recorded. In this case,
> lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
> 
> Previously, these situations were ignored and the profiler
> reported, that the source line was equal to zero.
> 
> This patch adjusts profiler behavior to treat allocations
> described above as internal by dumping ASOURCE_INT if
> lj_debug_frameline() returns a negative value.
> 
> Resolves tarantool/tarantool#5679
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/5679
> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
> 
> Changes in v2:
>   - Fixed commit title to fit in width of 50 symbols;
>   - Rebased to the branch [1];
>   - Reused default_payload() as a payload to test patch behavior.
> 
> [1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
> 
>  src/lj_memprof.c                              | 28 ++++++++++++-------
>  .../misclib-memprof-lapi.test.lua             | 16 ++++++++---
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
> index 87512c3a..e4d819a0 100644
> --- a/src/lj_memprof.c
> +++ b/src/lj_memprof.c

<snipped>

> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index b7e456e1..96864c6a 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>  local tap = require("tap")
>  
>  local test = tap.test("misc-memprof-lapi")
> -test:plan(16)
> +test:plan(17)
>  
>  jit.off()
>  jit.flush()
> @@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
>    for _, event in pairs(events[event_type]) do
>      local addr = event.loc.addr
>      local traceno = event.loc.traceno
> -    if traceno ~= 0 then
> +    if traceno ~= 0 and symbols.trace[traceno] then

This should be moved to the commit, that changes symtab, this change
looks unrelated to this patch, but relates to previous one [1].

>        local trace_loc = symbols.trace[traceno]
>        addr = trace_loc.addr
>        ev_type[trace_loc.line] = {
> @@ -214,9 +214,17 @@ misc.memprof.stop()

<snipped>

> -- 
> 2.32.0
> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/024944.html

-- 
Best regards,
Sergey Kaplun

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

* Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal
  2021-08-14 11:24 ` Sergey Kaplun via Tarantool-patches
@ 2021-08-21  4:43   ` Mikhail Shishatskiy via Tarantool-patches
  2021-11-24 13:01     ` Mikhail Shishatskiy via Tarantool-patches
  0 siblings, 1 reply; 4+ messages in thread
From: Mikhail Shishatskiy via Tarantool-patches @ 2021-08-21  4:43 UTC (permalink / raw)
  To: Sergey Kaplun, tarantool-patches, imun

Hi, Sergey!
Thanks for the review!

On 14.08.2021 14:24, Sergey Kaplun wrote:
>Hi, thanks for the fixes!
>
>Please, move test adjustment mentioned below to the patch for the symtab
>with group allocations by trace's number.
>Otherwise, LGTM.
>
>On 29.07.21, Mikhail Shishatskiy wrote:
>> There are cases when the memory profiler attempts to attribute
>> allocations triggered by the JIT engine recording phase
>> with a Lua function to be recorded. In this case,
>> lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
>>
>> Previously, these situations were ignored and the profiler
>> reported, that the source line was equal to zero.
>>
>> This patch adjusts profiler behavior to treat allocations
>> described above as internal by dumping ASOURCE_INT if
>> lj_debug_frameline() returns a negative value.
>>
>> Resolves tarantool/tarantool#5679
>> ---
>>
>> Issue: https://github.com/tarantool/tarantool/issues/5679
>> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>>
>> Changes in v2:
>> - Fixed commit title to fit in width of 50 symbols;
>> - Rebased to the branch [1];
>> - Reused default_payload() as a payload to test patch behavior.
>>
>> [1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
>>
>> src/lj_memprof.c | 28 ++++++++++++-------
>> .../misclib-memprof-lapi.test.lua | 16 ++++++++---
>> 2 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
>> index 87512c3a..e4d819a0 100644
>> --- a/src/lj_memprof.c
>> +++ b/src/lj_memprof.c
>
><snipped>
>
>> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> index b7e456e1..96864c6a 100644
>> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> @@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>> local tap = require("tap")
>>
>> local test = tap.test("misc-memprof-lapi")
>> -test:plan(16)
>> +test:plan(17)
>>
>> jit.off()
>> jit.flush()
>> @@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
>> for _, event in pairs(events[event_type]) do
>> local addr = event.loc.addr
>> local traceno = event.loc.traceno
>> - if traceno ~= 0 then
>> + if traceno ~= 0 and symbols.trace[traceno] then
>
>This should be moved to the commit, that changes symtab, this change
>looks unrelated to this patch, but relates to previous one [1].

Fixed in the patchset [1] and rebased to the branch [2]

>
>> local trace_loc = symbols.trace[traceno]
>> addr = trace_loc.addr
>> ev_type[trace_loc.line] = {
>> @@ -214,9 +214,17 @@ misc.memprof.stop()
>
><snipped>
>
>> --
>> 2.32.0
>>
>
>[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/024944.html
>
>--
>Best regards,
>Sergey Kaplun

Also, I slightly changed the test by deleting the check for deallocations
number because now trace generation happens for memprof payload, too, and
we can meet untrivial JIT-related deallocations in the second run.

---
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index f84b6df0..17bcb2f1 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -231,19 +231,25 @@ test:test("jit-output", function(subtest)
     jit.opt.start(3, "hotloop=10")
     jit.flush()
   
- -- Pregenerate traces to fill symtab entries in the next run.
- default_payload()
-
+ -- On this run traces are generated, JIT-related allocations
+ -- will be recorded as well.
     local symbols, events = generate_parsed_output(default_payload)
   
     local alloc = fill_ev_type(events, symbols, "alloc")
- local free = fill_ev_type(events, symbols, "free")
+
+ -- Test for marking JIT-related allocations as internal.
+ -- See also https://github.com/tarantool/tarantool/issues/5679.
+ subtest:ok(alloc[0] == nil)
+
+ -- Run already generated traces.
+ symbols, events = generate_parsed_output(default_payload)
+
+ alloc = fill_ev_type(events, symbols, "alloc")
   
     -- We expect, that loop will be compiled into a trace.
     subtest:ok(check_alloc_report(alloc, 1, 37, 32, 20))
     -- See same checks with jit.off().
     subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2))
- subtest:ok(free.INTERNAL.num == 22)
   
     -- Restore default JIT settings.
     jit.opt.start(unpack(jit_opt_default))
--
2.32.0

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025680.html
[2]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number

Issue: https://github.com/tarantool/tarantool/issues/5679
Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
Tarantool branch: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal

Best regards,
Mikhail Shishatskiy

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

* Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal
  2021-08-21  4:43   ` Mikhail Shishatskiy via Tarantool-patches
@ 2021-11-24 13:01     ` Mikhail Shishatskiy via Tarantool-patches
  0 siblings, 0 replies; 4+ messages in thread
From: Mikhail Shishatskiy via Tarantool-patches @ 2021-11-24 13:01 UTC (permalink / raw)
  To: Mikhail Shishatskiy; +Cc: tarantool-patches

Rebased to the updated gh-5814 [1] branch.

@ChangeLog
======================================================================
##feature/luajit

* Now memory profiler reports JIT-related allocations, which are not
   connected to a particular trace, as INTERNAL.
======================================================================

[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number

On 21.08.2021 11:43, Mikhail Shishatskiy via Tarantool-patches wrote:
>Hi, Sergey!
>Thanks for the review!
>
>On 14.08.2021 14:24, Sergey Kaplun wrote:
>>Hi, thanks for the fixes!
>>
>>Please, move test adjustment mentioned below to the patch for the symtab
>>with group allocations by trace's number.
>>Otherwise, LGTM.
>>
>>On 29.07.21, Mikhail Shishatskiy wrote:
>>>There are cases when the memory profiler attempts to attribute
>>>allocations triggered by the JIT engine recording phase
>>>with a Lua function to be recorded. In this case,
>>>lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
>>>
>>>Previously, these situations were ignored and the profiler
>>>reported, that the source line was equal to zero.
>>>
>>>This patch adjusts profiler behavior to treat allocations
>>>described above as internal by dumping ASOURCE_INT if
>>>lj_debug_frameline() returns a negative value.
>>>
>>>Resolves tarantool/tarantool#5679
>>>---
>>>
>>>Issue: https://github.com/tarantool/tarantool/issues/5679
>>>Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>>>CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>>>
>>>Changes in v2:
>>>- Fixed commit title to fit in width of 50 symbols;
>>>- Rebased to the branch [1];
>>>- Reused default_payload() as a payload to test patch behavior.
>>>
>>>[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
>>>
>>>src/lj_memprof.c | 28 ++++++++++++-------
>>>.../misclib-memprof-lapi.test.lua | 16 ++++++++---
>>>2 files changed, 30 insertions(+), 14 deletions(-)
>>>
>>>diff --git a/src/lj_memprof.c b/src/lj_memprof.c
>>>index 87512c3a..e4d819a0 100644
>>>--- a/src/lj_memprof.c
>>>+++ b/src/lj_memprof.c
>>
>><snipped>
>>
>>>diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>>>index b7e456e1..96864c6a 100644
>>>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>>>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>>>@@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>>>local tap = require("tap")
>>>
>>>local test = tap.test("misc-memprof-lapi")
>>>-test:plan(16)
>>>+test:plan(17)
>>>
>>>jit.off()
>>>jit.flush()
>>>@@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
>>>for _, event in pairs(events[event_type]) do
>>>local addr = event.loc.addr
>>>local traceno = event.loc.traceno
>>>- if traceno ~= 0 then
>>>+ if traceno ~= 0 and symbols.trace[traceno] then
>>
>>This should be moved to the commit, that changes symtab, this change
>>looks unrelated to this patch, but relates to previous one [1].
>
>Fixed in the patchset [1] and rebased to the branch [2]
>
>>
>>>local trace_loc = symbols.trace[traceno]
>>>addr = trace_loc.addr
>>>ev_type[trace_loc.line] = {
>>>@@ -214,9 +214,17 @@ misc.memprof.stop()
>>
>><snipped>
>>
>>>--
>>>2.32.0
>>>
>>
>>[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/024944.html
>>
>>--
>>Best regards,
>>Sergey Kaplun
>
>Also, I slightly changed the test by deleting the check for deallocations
>number because now trace generation happens for memprof payload, too, and
>we can meet untrivial JIT-related deallocations in the second run.
>
>---
>diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>index f84b6df0..17bcb2f1 100644
>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>@@ -231,19 +231,25 @@ test:test("jit-output", function(subtest)
>    jit.opt.start(3, "hotloop=10")
>    jit.flush()
>- -- Pregenerate traces to fill symtab entries in the next run.
>- default_payload()
>-
>+ -- On this run traces are generated, JIT-related allocations
>+ -- will be recorded as well.
>    local symbols, events = generate_parsed_output(default_payload)
>    local alloc = fill_ev_type(events, symbols, "alloc")
>- local free = fill_ev_type(events, symbols, "free")
>+
>+ -- Test for marking JIT-related allocations as internal.
>+ -- See also https://github.com/tarantool/tarantool/issues/5679.
>+ subtest:ok(alloc[0] == nil)
>+
>+ -- Run already generated traces.
>+ symbols, events = generate_parsed_output(default_payload)
>+
>+ alloc = fill_ev_type(events, symbols, "alloc")
>    -- We expect, that loop will be compiled into a trace.
>    subtest:ok(check_alloc_report(alloc, 1, 37, 32, 20))
>    -- See same checks with jit.off().
>    subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2))
>- subtest:ok(free.INTERNAL.num == 22)
>    -- Restore default JIT settings.
>    jit.opt.start(unpack(jit_opt_default))
>--
>2.32.0
>
>[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025680.html
>[2]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
>
>Issue: https://github.com/tarantool/tarantool/issues/5679
>Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>Tarantool branch: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>
>Best regards,
>Mikhail Shishatskiy

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

end of thread, other threads:[~2021-11-24 13:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29  9:34 [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal Mikhail Shishatskiy via Tarantool-patches
2021-08-14 11:24 ` Sergey Kaplun via Tarantool-patches
2021-08-21  4:43   ` Mikhail Shishatskiy via Tarantool-patches
2021-11-24 13:01     ` Mikhail Shishatskiy via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git