Re: [PATCH v3 6/6] scripts: python: implement get or create frame and stack function

[Ian Rogers]

On Mon, Jul 10, 2023 at 4:15 PM Anup Sharma <anupnewsmail@xxxxxxxxx> wrote:
>
> Complete addSample function, it takes the thread name, stack array,
> and time as input parameters and  if the thread name differs from
> the current name, it updates the name. The function utilizes the
> get_or_create_stack and get_or_create_frame methods to construct
> the stack structure. Finally, it adds the stack, time, and
> responsiveness values to the 'data' list within 'samples'.
>
> The get_or_create_stack function is responsible for retrieving
> or creating a stack based on the provided frame and prefix.
> It first generates a key using the frame and prefix values.
> If the stack corresponding to the key is found in the stackMap,
> it is returned. Otherwise, a new stack is created by appending
> the prefix and frame to the stackTable's 'data' array. The key
> and the index of the newly created stack are added to the
> stackMap for future reference.
>
> The get_or_create_frame function is responsible for retrieving or
> creating a frame based on the provided frameString. If the frame
> corresponding to the frameString is found in the frameMap, it is
> returned. Otherwise, a new frame is created by appending relevant
> information to the frameTable's 'data' array and adding the
> frameString to the stringTable.
>
> Signed-off-by: Anup Sharma <anupnewsmail@xxxxxxxxx>
> ---
>  .../scripts/python/firefox-gecko-converter.py | 57 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> index 6c934de1f608..97fbe562ee2b 100644
> --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> @@ -21,6 +21,8 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
>  from perf_trace_context import *
>  from Core import *
>
> +USER_CATEGORY_INDEX = 0
> +KERNEL_CATEGORY_INDEX = 1
>  thread_map = {}
>  start_time = None
>
> @@ -34,7 +36,12 @@ CATEGORIES = [
>  PRODUCT = os.popen('uname -op').read().strip()
>
>  def trace_end():
> -    thread_array = thread_map.values()))
> +    thread_array = list(map(lambda thread: thread['finish'](), thread_map.values()))

With a class this will be a more intuitive:

thread.finish()

> +
> +# Parse the callchain of the current sample into a stack array.
> +    for thread in thread_array:
> +        key = thread['samples']['schema']['time']

I'm not sure what 'schema' is here. Worth a comment.

> +        thread['samples']['data'].sort(key=lambda data : float(data[key]))

Perhaps there is a more intention revealing name than "data".

>
>      result = {
>          'meta': {
> @@ -106,7 +113,55 @@ def process_event(param_dict):
>                 }
>                 stringTable = []
>
> +               stackMap = dict()
> +               def get_or_create_stack(frame, prefix):

Can you comment what frame and prefix are with examples, otherwise
understanding this function is hard.

> +                       key = f"{frame}" if prefix is None else f"{frame},{prefix}"
> +                       stack = stackMap.get(key)
> +                       if stack is None:
> +                               stack = len(stackTable['data'])
> +                               stackTable['data'].append([prefix, frame])
> +                               stackMap[key] = stack
> +                       return stack
> +
> +               frameMap = dict()
> +               def get_or_create_frame(frameString):

s/frameMap/frame_map/
s/frameString/frame_string/

These variable names aren't going a long way to helping understand the
code. They mention frame and then the type, which should really be
type information like ": Dict[...]". Can you improve the names as
otherwise we effectively have 3 local variables all called "frame".

> +                       frame = frameMap.get(frameString)
> +                       if frame is None:
> +                               frame = len(frameTable['data'])
> +                               location = len(stringTable)
> +                               stringTable.append(frameString)
> +                               category = KERNEL_CATEGORY_INDEX if frameString.find('kallsyms') != -1 \
> +                                               or frameString.find('/vmlinux') != -1 \
> +                                               or frameString.endswith('.ko)') \
> +                                               else USER_CATEGORY_INDEX

Perhaps make this a helper function, symbol_name_to_category_index.

> +                               implementation = None
> +                               optimizations = None
> +                               line = None
> +                               relevantForJS = False

Some comments on these variables would be useful, perhaps just use
named arguments below.

> +                               subcategory = None
> +                               innerWindowID = 0
> +                               column = None
> +
> +                               frameTable['data'].append([
> +                                       location,
> +                                       relevantForJS,
> +                                       innerWindowID,
> +                                       implementation,
> +                                       optimizations,
> +                                       line,
> +                                       column,
> +                                       category,
> +                                       subcategory,
> +                               ])
> +                               frameMap[frameString] = frame
> +                       return frame
> +
>                 def addSample(threadName, stackArray, time):
> +                       nonlocal name
> +                       if name != threadName:
> +                               name = threadName

A comment/example would be useful here.

> +                       stack = reduce(lambda prefix, stackFrame: get_or_create_stack
> +                                       (get_or_create_frame(stackFrame), prefix), stackArray, None)

Thanks,
Ian

>                         responsiveness = 0
>                         samples['data'].append([stack, time, responsiveness])
>
> --
> 2.34.1
>