Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mikhail Elhimov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>,
	Mikhail Elhimov <elhimov@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack
Date: Tue, 23 Aug 2022 12:13:59 +0300	[thread overview]
Message-ID: <d1bb1d43-72c0-655c-cc80-730d8abf0267@vk.team> (raw)
In-Reply-To: <YwB+8nY5SOKwVES6@root>

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

Hi, Sergey!

Thanks for the review!

On 20.08.2022 09:28, Sergey Kaplun wrote:
> Hi, Mikhail!
>
> Thanks for the patch!
>
> It's generally LGTM, except a bunch of nits and typos.
> BTW, I like your iterator approach: the code looks so much better now!
>
> On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
>> Changes:
>> - Introduce generator to iterate over frames in a Python way
>> - Dump framelink of the bottom frame in a common way and identify it as
>>    a dummy framelink only inside of common dump function
> Typo: s/of common/of the common/
Fixed
>> - Framelink always refers to the slot right before frame's base
> Don't get this change bullet description. Looks like this is just the
> way you implement frame iterator, isn't it?

Fixed

Yes, this bullet described both "what" and "how" of the change. I turned 
this bullet into "Iterate over frames in a Python way (with iterator)"

>> - Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic
>>    more obvious. Previous using of boolean LJ_RT2 in arithmetic looked
> Typo: s/LJ_RT2/LJ_FR2
Fixed
>>    like a tricky way, both to read and maintain (however places where
> Side note: As for me it looks like obvious and natural :). But maybe
> this is the ill effect from LuaJIT sources. I leave comments in places
> where I suggest to use LJ_FR2 instead LJ_FRAMELINK_NUM_SLOTS.

My point is: LJ_FR2 -- for boolean context, LJ_FRAMELINK_NUM_SLOTS -- 
for formulas

Let me share my considerations.

I see LJ_FR2 as a boolean entity that means either 1 or 2 slots are used 
to store "framelink" and since it is boolean, the only natural usage for 
it from my point of view is boolean context (i.e. if ... else ...). I 
understand that using boolean in formulas reduces branching and helps to 
improve performance, but here it doesn't seem to be necessary. For me 
operating with "number of slots" is easier than with "number of extra 
slots over 1" (which LJ_FR2 turns into in formulas). The latter is ok 
when it's standalone, but when it appears in formula I feel that I need 
to perform some additional calculation in my mind while reading code, 
that's why I find it less-readable and introduced LJ_FRAMELINK_NUM_SLOTS 
and that's why I would keep using LJ_FRAMELINK_NUM_SLOTS in formulas, 
and wouldn't turn back LJ_FR2 even when `LJ_FRAMELINK_NUM_SLOTS - 1` occurs

An example:

red = 5 + 2 * LJ_FR2

which looks a bit magic, turns into

red = 3 + 2 * LJ_FRAMELINK_NUM_SLOTS

which produce the same results, but literally says that we reserve 3 
data slots for 2 frames (I just realized that I forgot to ask the 
confirmation regarding my interpretation of the number of slots reserved 
and left the original formula intact, so if it's correct I'd adjust it)

These considerations may be changed when I dive deeper into luajit, but 
for now they are ))

>>    LJ_RT2 approach had been originally replicated from the source code
> Typo: s/LJ_RT2/LJ_FR2
Fixed
>>    are left intact, because it is expected that maintenance efforts for
>>    them will be minimal)
> General: Missed dot at the end of the sentences.
Fixed
>> ---
>>   src/luajit-gdb.py | 101 ++++++++++++++++++++++------------------------
>>   1 file changed, 49 insertions(+), 52 deletions(-)
>>
>> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>> index 1e9a96fb..d7d34c70 100644
>> --- a/src/luajit-gdb.py
>> +++ b/src/luajit-gdb.py
>> @@ -158,6 +158,7 @@ LJ_64 = None
> <snipped>
>
>> @@ -299,6 +300,21 @@ gclen = {
>>       'mmudata': gcringlen,
>>   }
>>   
>> +def get_framelink_sentinel(L):
>> +    stack = mref('TValue *', L['stack'])
>> +    return stack + LJ_FRAMELINK_NUM_SLOTS - 1
> `LJ_FRAMELINK_NUM_SLOTS - 1 == LJ_FR2` so we can replace it like the
> following:
> | return stack + LJ_FR2
>
>> +
>> +# Generator that implements frame iterator
> Typo: s/Generator/The generator/
> Typo: s/iterator/iterator./
Fixed
>> +# every frame is represented as a tuple of framelink and frametop
> Typo: s/every/Every/
> Typo: s/frametop/frametop./
Fixed
> Minor: I suggest to specify that this is yielded to the caller tuple.
> Feel free to ignore.
Sorry, I didn't catch the suggestion. Would you, please, explain?
>> +def frames(L):
>> +    frametop = L['top']
>> +    framelink = L['base'] - 1
>> +    framelink_sentinel = get_framelink_sentinel(L)
>> +    while framelink is not None:
> Minor: `is not None` is excess, we can just omit it.

I'd prefer to follow the recommendation from PEP8 
<https://peps.python.org/pep-0008/#programming-recommendations>

=========

Also, beware of writing if x when you really mean if x is not None – 
e.g. when testing whether a variable or argument that defaults to None 
was set to some other value. The other value might have a type (such as 
a container) that could be false in a boolean context!

=========

However while checking this code I found 2-step loop exit: first, inside 
the loop we check that sentinel is achieved, assign `None` and catch it 
immediately at checking loop condition. I adjusted loop as follows:

|while True:||
||        yield framelink, frametop||
||        frametop = framelink - LJ_FRAMELINK_NUM_SLOTS||
||        if framelink <= framelink_sentinel:||
||            break||
||        framelink = frame_prev(framelink)||
|

so finally `is not None` was removed, but for another reason )

>> +        yield framelink, frametop
>> +        frametop = framelink - LJ_FRAMELINK_NUM_SLOTS
>> +        framelink = frame_prev(framelink) if framelink > framelink_sentinel else None
>> +
>>   # Dumpers {{{
>>   
>>   def dump_lj_tnil(tv):
>> @@ -397,32 +413,38 @@ dumpers = {
>>   def dump_tvalue(tvalue):
>>       return dumpers.get(typenames(itypemap(tvalue)), dump_lj_invalid)(tvalue)
>>   
>> +def dump_framelink_slot_address(fr):
>> +    return '{}:{}'.format(fr - 1, fr) if LJ_FR2 else '{}'.format(fr) + PADDING
>> +
>> +def dump_framelink_func(fr):
>> +    return dump_lj_tfunc(fr - 1 if LJ_FR2 else fr)
> We can replace it with the following:
> | return dump_lj_tfunc(fr - LJ_FR2)
Please, see my consideration above
>> +
>>   def dump_framelink(L, fr):
> <snipped>
>
>>       )
>>   
>> -def dump_stack_slot(L, slot, base=None, top=None, eol='\n'):
>> +def dump_stack_slot(L, slot, base=None, top=None):
> <snipped>
>
>>   
>>   def dump_stack(L, base=None, top=None):
> <snipped>
>
>> +    for framelink, frametop in frames(L):
>> +        # dump all data slots in the (framelink, top) interval
> Typo: s/dump/Dump/
> Typo: s/interval/interval./
Fixed
>> +        dump.extend([
>> +            dump_stack_slot(L, framelink + offset, base, top)
>> +                for offset in range(frametop - framelink, 0, -1)
>> +        ])
>> +        # dump frame slot (2 slots in case of GC64)
> Typo: s/dump/Dump/
> Missed dot at the end of the sentence.
Fixed
>> +        dump.append( dump_framelink(L, framelink) )
>> +
>> +    return '\n'.join(dump)
>>   
>>   def dump_gc(g):
>>       gc = g['gc']
>> @@ -717,7 +713,7 @@ The command requires no args and dumps current GC stats:
> <snipped>
>
>> @@ -759,6 +755,7 @@ def init(commands):
>>           LJ_64 = str(gdb.parse_and_eval('IRT_PTR')) == 'IRT_P64'
>>           LJ_FR2 = LJ_GC64 = str(gdb.parse_and_eval('IRT_PGC')) == 'IRT_P64'
>>           LJ_DUALNUM = gdb.lookup_global_symbol('lj_lib_checknumber') is not None
>> +        LJ_FRAMELINK_NUM_SLOTS = 2 if LJ_FR2 else 1
> As far as LJ_FR2 is defined before, we can initialize this variable like
> the following:
> | LJ_FRAMELINK_NUM_SLOTS = LJ_FR2 + 1
Please, see my consideration above
>>       except:
>>           gdb.write('luajit-gdb.py failed to load: '
>>                     'no debugging symbols found for libluajit\n')
>> -- 
>> 2.34.1
>>
The corrected commit message:
===================================================
gdb: refactor iteration over frames while dumping stack

Changes:
- Iterate over frames in a Python way (with iterator)
- Dump framelink of the bottom frame in a common way and identify it as
   a dummy framelink only inside of the common dump function
- Framelink always refers to the slot right before frame's base
- Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic
   more obvious. Previous using of boolean LJ_FR2 in arithmetic looked
   like a tricky way, both to read and maintain (however places where
   LJ_FR2 approach had been originally replicated from the source code
   are left intact, because it is expected that maintenance efforts for
   them will be minimal).

===================================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index d7d34c70..2c11057b 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -304,16 +304,18 @@ def get_framelink_sentinel(L):
      stack = mref('TValue *', L['stack'])
      return stack + LJ_FRAMELINK_NUM_SLOTS - 1
  
-# Generator that implements frame iterator
-# every frame is represented as a tuple of framelink and frametop
+# The generator that implements frame iterator.
+# Every frame is represented as a tuple of framelink and frametop.
  def frames(L):
      frametop = L['top']
      framelink = L['base'] - 1
      framelink_sentinel = get_framelink_sentinel(L)
-    while framelink is not None:
+    while True:
          yield framelink, frametop
          frametop = framelink - LJ_FRAMELINK_NUM_SLOTS
-        framelink = frame_prev(framelink) if framelink > framelink_sentinel else None
+        if framelink <= framelink_sentinel:
+            break
+        framelink = frame_prev(framelink)
  
  # Dumpers {{{
  
@@ -477,12 +479,12 @@ def dump_stack(L, base=None, top=None):
      ])
  
      for framelink, frametop in frames(L):
-        # dump all data slots in the (framelink, top) interval
+        # Dump all data slots in the (framelink, top) interval.
          dump.extend([
              dump_stack_slot(L, framelink + offset, base, top)
                  for offset in range(frametop - framelink, 0, -1)
          ])
-        # dump frame slot (2 slots in case of GC64)
+        # Dump frame slot (2 slots in case of GC64).
          dump.append( dump_framelink(L, framelink) )
  
      return '\n'.join(dump)
===================================================

-- 
Best regards,
Mikhail Elhimov

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

  reply	other threads:[~2022-08-23  9:14 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-23  0:11 [Tarantool-patches] [PATCH 0/2] gdb: adjust to support Python 2 + refactoring of stack dumping Mikhail Elhimov via Tarantool-patches
2022-07-23  0:11 ` [Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7) Mikhail Elhimov via Tarantool-patches
2022-08-20  6:26   ` Sergey Kaplun via Tarantool-patches
2022-08-22 13:00     ` Mikhail Elhimov via Tarantool-patches
2022-08-22 17:46     ` Mikhail Elhimov via Tarantool-patches
2022-08-31 14:47   ` Igor Munkin via Tarantool-patches
2022-08-31 23:17     ` Mikhail Elhimov via Tarantool-patches
2022-07-23  0:11 ` [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack Mikhail Elhimov via Tarantool-patches
2022-08-20  6:28   ` Sergey Kaplun via Tarantool-patches
2022-08-23  9:13     ` Mikhail Elhimov via Tarantool-patches [this message]
2022-08-31 14:47   ` Igor Munkin via Tarantool-patches
2022-08-31 23:20     ` Mikhail Elhimov via Tarantool-patches
2022-09-14 21:28       ` Igor Munkin via Tarantool-patches
2022-11-11  8:53 ` [Tarantool-patches] [PATCH 0/2] gdb: adjust to support Python 2 + refactoring of stack dumping Igor Munkin 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=d1bb1d43-72c0-655c-cc80-730d8abf0267@vk.team \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=elhimov@gmail.com \
    --cc=m.elhimov@vk.team \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack' \
    /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