[Bug tapsets/16472] New: long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[Bug tapsets/16472] New: long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[fahadaliarshad at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

            Bug ID: 16472
           Summary: long chain of nested function calls consumes a lot of
                    empty whitespace due to thread_indent() function
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: tapsets
          Assignee: systemtap at sourceware dot org
          Reporter: fahadaliarshad at gmail dot com

Created attachment 7365
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7365&action=edit
patch for add_thread_indent_depth() function

This function (thread_indent_depth) is useful for saving space (consumed by
whitespace) in traces with long nested function calls. It returns an integer
(represented as a string) corresponding to the nested-depth in the current task
information.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[fahadaliarshad at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

--- Comment #1 from fahadaliarshad at gmail dot com ---
Is there any chance of getting this patch into systemtap? 

While tracing the function-call entries only, the size of the generated file
gets extra-ordinarily large (due to long nestations and no returns being
probed). The function thread_indent(long) which prints whitespace for
pretty-printing needs a sibling(thread_indent_depth(long)) that only prints the
nestation-depth as number as oppose to whitespace. Availability of
thread_indent_depth(long) in native
tapsets(/usr/share/systemtap/tapset/indent.stp) would provide any systemtap
user much more flexibility.

Thanks.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

Josh Stone <jistone at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jistone at redhat dot com

--- Comment #2 from Josh Stone <jistone at redhat dot com> ---
I think that only the nesting-depth is not enough.  This is thread-specific, so
it ought to have the same execname/tid identification as thread_indent().  The
tapset should be consistent with providing timestamps too.

So, I propose a new parameter flag to _generic_indent() to choose whether to
use the old indented style ("%-*s", x, "") or a new numeric style something
like ("[%d] ", x).  Then thread_indent_num() would look exactly like
thread_indent() except for choosing the new style, and we should also have an
indent_num() mirroring indent().

I renamed to _num because I suspect you may still want something that just
returns the depth.  If so, we can also factor out the _indent_counters[]
manipulation into something like _generic_indent_depth(idx, delta) returning
the number.  Then _generic_indent would call this, as would
thread_indent_depth() and indent_depth() to return numbers to the user.

Now I think my _num/_depth naming might be more confusing; let's think about
what to call them.  One version which prints the number instead of spacing,
with the same timestamp/execname/tid formatting, and another version which
returns the raw number for the caller to use as they please.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

--- Comment #3 from Josh Stone <jistone at redhat dot com> ---
On further reflection, I'm probably complicated your request too much.  I think
it would be fine to simply factor out a _generic_indent_depth(idx,delta) and
base your thread_indent_depth:long(delta:long) on that - but note I'm
suggesting to return the number directly.  That's more flexible, and the caller
can easily make that a string if desired.  We should have a global
indent_depth() too.

Note this should also maintain the post-increment/pre-decrement behavior, which
is desirable to keep +1/-1 pairs (like call/return) reported at the same level.
 Your original patch always reported the resulting indentation, which means a
call would report greater indentation than its return.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[fahadaliarshad at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

--- Comment #4 from fahadaliarshad at gmail dot com ---
I agree. Having thread_indent_depth(delta:long) return the number directly (as
oppose to string) is better and more flexible.

You are correct on the post-increment/pre-decrement call/return behavior.
Although, if this difference by one for call/return is consistent (which it
would be) across all entry/returns then it does not matter. But for lesser
confusion, lets keep the call/return reported at the same level as you
suggested. 

Thanks for your help. Do you have any idea when this might be merged?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

--- Comment #5 from Josh Stone <jistone at redhat dot com> ---
(In reply to fahadaliarshad from comment #4)
> Thanks for your help. Do you have any idea when this might be merged?

Do you care to write an updated patch from our discussion?
If not, I can try to do it myself in the next few days.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[fahadaliarshad at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

--- Comment #6 from fahadaliarshad at gmail dot com ---
Sure, I will try to work on the updated patch (as per our discussion) this
weekend. I will get back to you soon. Thanks.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[fahadaliarshad at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

fahadaliarshad at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7365|0                           |1
        is obsolete|                            |

--- Comment #7 from fahadaliarshad at gmail dot com ---
Created attachment 7439
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7439&action=edit
patch (ver 2) for adding call-depth functionality

Based on your comment #3, I have factored out _generic_indent_depth:long (idx,
delta) and based my thread_indent_depth:long (delta:long) on it, which returns
a number now. Also, I tested the post-increment/pre-decrement behavior to keep
+1/-1 pairs (call/return) consistent.

Below is a sample output from one of my applications. Note that column two
represent the function call-depth. The field next to calls/returns(=>/<=) is
the called/returned-from function-name.

1393423651768235 0 => import
1393423651768554 1 => main
1393423651768567 1 <= main
1393423651768579 0 <= import
1393423651768594 0 => loadClass
1393423651768602 1 => findFile
1393423651768612 1 <= findFile
1393423651770492 1 => main
1393423651770509 1 <= main
1393423651770517 0 <= loadClass
1393423651770528 0 => getApplication
1393423651770544 1 => loadClass
1393423651770551 2 => findFile
1393423651770562 2 <= findFile

Please do let me know if this is fine for merging or is there anything else you
suggest to add. Thanks.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

Josh Stone <jistone at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Josh Stone <jistone at redhat dot com> ---
Thanks, I've committed your patch.  I also made a few small additions:
- Add indent_depth() as a global analogue of thread_indent_depth()
- Reuse _generic_indent_depth() from _generic_indent() too.
- Expand buildok/indent.stp for all indentation functions.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug tapsets/16472] long chain of nested function calls consumes a lot of empty whitespace due to thread_indent() function

[fahadaliarshad at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16472

--- Comment #9 from fahadaliarshad at gmail dot com ---
Great. Thanks.

-- 
You are receiving this mail because:
You are the assignee for the bug.