[PATCH v3 0/6] Add support for Firefox's gecko profile format

[PATCH v3 0/6] Add support for Firefox's gecko profile format

[Anup Sharma]

This patch series adds support for Firefox's gecko profile format.
The format is documented here [1].

I have incorporated several changes based on feedback from the
previous version of the patch. However, there are still a few
remaining comments that I am currently addressing.

Additionally, I am still in the process of learning how to send
patches in a series, so there may be some shortcomings in this
particular series as well.

changes from v2:
- renamed mod to func
- removed unnessecary imports print_function
- removed shebang python env declaration
- changed _createtread to _createThread
- Commits in better order.

[1] https://github.com/firefox-devtools/profiler/blob/main/docs-developer/gecko-profile-format.md

Anup Sharma (6):
  scripts: python: Add initial script file with imports.
  scripts: python: Extact necessary information from process event
  scripts: python: thread sample processing to create thread with
    schemas
  scripts: python: Add trace end processing and JSON output
  scripts: python: Implement add sample function and return finish
  scripts: python: implement get or create frame and stack function

 .../scripts/python/firefox-gecko-converter.py | 221 ++++++++++++++++++
 1 file changed, 221 insertions(+)
 create mode 100644 tools/perf/scripts/python/firefox-gecko-converter.py

-- 
2.34.1

[PATCH v3 1/6] scripts: python: Add initial script file with imports

[Anup Sharma]

Added the necessary modules, including the Perf-Trace-Util
library, and defines the required functions and variables.
It leverages the perf_trace_context and Core modules for
tracing and processing events. Also added usage information.

Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
---
 .../scripts/python/firefox-gecko-converter.py | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 tools/perf/scripts/python/firefox-gecko-converter.py

diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
new file mode 100644
index 000000000000..5b342641925c
--- /dev/null
+++ b/tools/perf/scripts/python/firefox-gecko-converter.py
@@ -0,0 +1,28 @@
+# firefox-gecko-converter.py - Convert perf record output to Firefox's gecko profile format
+# SPDX-License-Identifier: GPL-2.0
+#
+# The script converts perf.data to Gecko Profile Format,
+# which can be read by https://profiler.firefox.com/.
+#
+# Usage:
+#
+#     perf record -a -g -F 99 sleep 60
+#     perf script firefox-gecko-converter.py > output.json
+
+import os
+import sys
+import json
+from functools import reduce
+
+# Add the Perf-Trace-Util library to the Python path
+sys.path.append(os.environ['PERF_EXEC_PATH'] + \
+	'/scripts/python/Perf-Trace-Util/lib/Perf/Trace')
+
+from perf_trace_context import *
+from Core import *
+
+def trace_end():
+	pass
+
+def process_event(param_dict):
+	pass
-- 
2.34.1

Re: [PATCH v3 1/6] scripts: python: Add initial script file with imports

[Ian Rogers]

On Mon, Jul 10, 2023 at 4:09 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
>
> Added the necessary modules, including the Perf-Trace-Util
> library, and defines the required functions and variables.
> It leverages the perf_trace_context and Core modules for
> tracing and processing events. Also added usage information.
>
> Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>

Acked-by: Ian Rogers <irogers@google.com>

> ---
>  .../scripts/python/firefox-gecko-converter.py | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 tools/perf/scripts/python/firefox-gecko-converter.py
>
> diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> new file mode 100644
> index 000000000000..5b342641925c
> --- /dev/null
> +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> @@ -0,0 +1,28 @@
> +# firefox-gecko-converter.py - Convert perf record output to Firefox's gecko profile format
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# The script converts perf.data to Gecko Profile Format,
> +# which can be read by https://profiler.firefox.com/.
> +#
> +# Usage:
> +#
> +#     perf record -a -g -F 99 sleep 60
> +#     perf script firefox-gecko-converter.py > output.json
> +
> +import os
> +import sys
> +import json
> +from functools import reduce
> +

nit: technically some of these imports should be added when necessary
in the code.

> +# Add the Perf-Trace-Util library to the Python path
> +sys.path.append(os.environ['PERF_EXEC_PATH'] + \
> +       '/scripts/python/Perf-Trace-Util/lib/Perf/Trace')
> +
> +from perf_trace_context import *
> +from Core import *
> +
> +def trace_end():
> +       pass
> +
> +def process_event(param_dict):

nit: (this is likely addressed in later patches) you can add return
and parameter types here to aid understanding of the code. Function
comments are also useful.

> +       pass
> --
> 2.34.1
>

Re: [PATCH v3 1/6] scripts: python: Add initial script file with imports

[Anup Sharma]

On Wed, Jul 12, 2023 at 09:50:40AM -0700, Ian Rogers wrote:
> On Mon, Jul 10, 2023 at 4:09 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
> >
> > Added the necessary modules, including the Perf-Trace-Util
> > library, and defines the required functions and variables.
> > It leverages the perf_trace_context and Core modules for
> > tracing and processing events. Also added usage information.
> >
> > Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> > ---
> >  .../scripts/python/firefox-gecko-converter.py | 28 +++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 tools/perf/scripts/python/firefox-gecko-converter.py
> >
> > diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> > new file mode 100644
> > index 000000000000..5b342641925c
> > --- /dev/null
> > +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> > @@ -0,0 +1,28 @@
> > +# firefox-gecko-converter.py - Convert perf record output to Firefox's gecko profile format
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# The script converts perf.data to Gecko Profile Format,
> > +# which can be read by https://profiler.firefox.com/.
> > +#
> > +# Usage:
> > +#
> > +#     perf record -a -g -F 99 sleep 60
> > +#     perf script firefox-gecko-converter.py > output.json
> > +
> > +import os
> > +import sys
> > +import json
> > +from functools import reduce
> > +
> 
> nit: technically some of these imports should be added when necessary
> in the code.

Noted.

> > +# Add the Perf-Trace-Util library to the Python path
> > +sys.path.append(os.environ['PERF_EXEC_PATH'] + \
> > +       '/scripts/python/Perf-Trace-Util/lib/Perf/Trace')
> > +
> > +from perf_trace_context import *
> > +from Core import *
> > +
> > +def trace_end():
> > +       pass
> > +
> > +def process_event(param_dict):
> 
> nit: (this is likely addressed in later patches) you can add return
> and parameter types here to aid understanding of the code. Function
> comments are also useful.

Noted.

> > +       pass
> > --
> > 2.34.1
> >

[PATCH v3 2/6] scripts: python: Extact necessary information from process event

[Anup Sharma]

The script takes in a sample event dictionary(param_dict) and retrieves
relevant data such as time stamp, PID, TID, thread name. Also start time
is defined.

Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
---
 .../perf/scripts/python/firefox-gecko-converter.py  | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
index 5b342641925c..765f1775cee5 100644
--- a/tools/perf/scripts/python/firefox-gecko-converter.py
+++ b/tools/perf/scripts/python/firefox-gecko-converter.py
@@ -21,8 +21,19 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
 from perf_trace_context import *
 from Core import *
 
+start_time = None
+
 def trace_end():
 	pass
 
 def process_event(param_dict):
-	pass
+	global start_time
+	# Extract relevant information from the event parameters. The event parameters
+	# are in a dictionary:
+	time_stamp = (param_dict['sample']['time'] // 1000) / 1000
+	pid = param_dict['sample']['pid']
+	tid = param_dict['sample']['tid']
+	thread_name = param_dict['comm']
+
+	# Assume that start time is the time of the first event.
+	start_time = time_stamp if not start_time else start_time
-- 
2.34.1

Re: [PATCH v3 2/6] scripts: python: Extact necessary information from process event

[Ian Rogers]

On Mon, Jul 10, 2023 at 4:10 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
>
> The script takes in a sample event dictionary(param_dict) and retrieves
> relevant data such as time stamp, PID, TID, thread name. Also start time
> is defined.
>
> Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> ---
>  .../perf/scripts/python/firefox-gecko-converter.py  | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> index 5b342641925c..765f1775cee5 100644
> --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> @@ -21,8 +21,19 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
>  from perf_trace_context import *
>  from Core import *
>

It'd be nice to have a comment here, perhaps:
# The time stamp from the first of the time ordered events.

> +start_time = None
> +
>  def trace_end():
>         pass
>
>  def process_event(param_dict):
> -       pass
> +       global start_time
> +       # Extract relevant information from the event parameters. The event parameters
> +       # are in a dictionary:
> +       time_stamp = (param_dict['sample']['time'] // 1000) / 1000
> +       pid = param_dict['sample']['pid']
> +       tid = param_dict['sample']['tid']
> +       thread_name = param_dict['comm']
> +
> +       # Assume that start time is the time of the first event.
> +       start_time = time_stamp if not start_time else start_time

I appreciate that this is one line, but it takes some getting your
head around that start_time is being assigned to itself in the common
case. I think this would be more readable as:

if not start_time:
  start_time = time_stamp

Thanks,
Ian

> --
> 2.34.1
>

Re: [PATCH v3 2/6] scripts: python: Extact necessary information from process event

[Ian Rogers]

On Wed, Jul 12, 2023 at 10:01 AM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Jul 10, 2023 at 4:10 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
> >
> > The script takes in a sample event dictionary(param_dict) and retrieves
> > relevant data such as time stamp, PID, TID, thread name. Also start time
> > is defined.
> >
> > Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> > ---
> >  .../perf/scripts/python/firefox-gecko-converter.py  | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> > index 5b342641925c..765f1775cee5 100644
> > --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> > +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> > @@ -21,8 +21,19 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
> >  from perf_trace_context import *
> >  from Core import *
> >
>
> It'd be nice to have a comment here, perhaps:
> # The time stamp from the first of the time ordered events.
>
> > +start_time = None
> > +
> >  def trace_end():
> >         pass
> >
> >  def process_event(param_dict):
> > -       pass
> > +       global start_time
> > +       # Extract relevant information from the event parameters. The event parameters
> > +       # are in a dictionary:
> > +       time_stamp = (param_dict['sample']['time'] // 1000) / 1000
> > +       pid = param_dict['sample']['pid']
> > +       tid = param_dict['sample']['tid']
> > +       thread_name = param_dict['comm']
> > +
> > +       # Assume that start time is the time of the first event.
> > +       start_time = time_stamp if not start_time else start_time
>
> I appreciate that this is one line, but it takes some getting your
> head around that start_time is being assigned to itself in the common
> case. I think this would be more readable as:
>
> if not start_time:
>   start_time = time_stamp

I believe the events are guaranteed time ordered in perf script. The
ordered_event logic handles this, but I likely haven't got a full
grasp on all the corners of it. You can always assert the behavior
(comments with guarantees :-) ):

assert start_time <= time_stamp, "Events aren't time ordered"

Thanks,
Ian

> Thanks,
> Ian
>
> > --
> > 2.34.1
> >

Re: [PATCH v3 2/6] scripts: python: Extact necessary information from process event

[Anup Sharma]

On Wed, Jul 12, 2023 at 10:03:57AM -0700, Ian Rogers wrote:
> On Wed, Jul 12, 2023 at 10:01 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Mon, Jul 10, 2023 at 4:10 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
> > >
> > > The script takes in a sample event dictionary(param_dict) and retrieves
> > > relevant data such as time stamp, PID, TID, thread name. Also start time
> > > is defined.
> > >
> > > Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> > > ---
> > >  .../perf/scripts/python/firefox-gecko-converter.py  | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> > > index 5b342641925c..765f1775cee5 100644
> > > --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> > > +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> > > @@ -21,8 +21,19 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
> > >  from perf_trace_context import *
> > >  from Core import *
> > >
> >
> > It'd be nice to have a comment here, perhaps:
> > # The time stamp from the first of the time ordered events.
> >

Sure. I will add it.

> > > +start_time = None
> > > +
> > >  def trace_end():
> > >         pass
> > >
> > >  def process_event(param_dict):
> > > -       pass
> > > +       global start_time
> > > +       # Extract relevant information from the event parameters. The event parameters
> > > +       # are in a dictionary:
> > > +       time_stamp = (param_dict['sample']['time'] // 1000) / 1000
> > > +       pid = param_dict['sample']['pid']
> > > +       tid = param_dict['sample']['tid']
> > > +       thread_name = param_dict['comm']
> > > +
> > > +       # Assume that start time is the time of the first event.
> > > +       start_time = time_stamp if not start_time else start_time
> >
> > I appreciate that this is one line, but it takes some getting your
> > head around that start_time is being assigned to itself in the common
> > case. I think this would be more readable as:
> >
> > if not start_time:
> >   start_time = time_stamp

Sure. I will change it.

> I believe the events are guaranteed time ordered in perf script. The
> ordered_event logic handles this, but I likely haven't got a full
> grasp on all the corners of it. You can always assert the behavior
> (comments with guarantees :-) ):
> 
> assert start_time <= time_stamp, "Events aren't time ordered"

I dont think assert is usefull here as start_time will be initialized
and assigned only once for all the process event triggers. So, the
significance of assert is not clear to me.

> Thanks,
> Ian
> 
> > Thanks,
> > Ian
> >
> > > --
> > > 2.34.1
> > >

[PATCH v3 3/6] scripts: python: thread sample processing to create thread with schemas

[Anup Sharma]

The _addThreadSample function is responsible for adding a sample
to a specific thread. It first checks if the thread exists in
the thread_map dictionary.

The markers structure defines the schema and data for
thread markers, including fields such as 'name',
'startTime', 'endTime', 'phase', 'category', and 'data'.

The samples structure defines the schema and data for thread
samples, including fields such as 'stack', 'time', and
'responsiveness'.

The frameTable structure defines the schema and data for frame
information, including fields such as 'location', 'relevantForJS',
'innerWindowID', 'implementation', 'optimizations', 'line',
'column', 'category', and 'subcategory'.

The purpose of this function is to create a new thread structure
These structures provide a framework for storing and organizing
information related to thread markers, samples, frame details,
and stack information.

The call stack is parsed to include function names and the associated
DSO, which are requires for creating json schema. Also few libaries
has been included which will be used in later commit.

Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
---
 .../scripts/python/firefox-gecko-converter.py | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
index 765f1775cee5..0b8a86bdcab1 100644
--- a/tools/perf/scripts/python/firefox-gecko-converter.py
+++ b/tools/perf/scripts/python/firefox-gecko-converter.py
@@ -21,6 +21,7 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
 from perf_trace_context import *
 from Core import *
 
+thread_map = {}
 start_time = None
 
 def trace_end():
@@ -28,6 +29,57 @@ def trace_end():
 
 def process_event(param_dict):
 	global start_time
+	global thread_map
+
+	def _createThread(name, pid, tid):
+		markers = {
+			'schema': {
+				'name': 0,
+				'startTime': 1,
+				'endTime': 2,
+				'phase': 3,
+				'category': 4,
+				'data': 5,
+			},
+			'data': [],
+		}
+		samples = {
+			'schema': {
+				'stack': 0,
+				'time': 1,
+				'responsiveness': 2,
+				},
+			'data': [],
+		}
+		frameTable = {
+			'schema': {
+				'location': 0,
+				'relevantForJS': 1,
+				'innerWindowID': 2,
+				'implementation': 3,
+				'optimizations': 4,
+				'line': 5,
+				'column': 6,
+				'category': 7,
+				'subcategory': 8,
+			},
+			'data': [],
+		}
+		stackTable = {
+			'schema': {
+				'prefix': 0,
+				'frame': 1,
+			},
+			'data': [],
+		}
+		stringTable = []
+
+	def _addThreadSample(pid, tid, threadName, time_stamp, stack):
+		thread = thread_map.get(tid)
+		if not thread:
+			thread = _createThread(threadName, pid, tid)
+			thread_map[tid] = thread
+
 	# Extract relevant information from the event parameters. The event parameters
 	# are in a dictionary:
 	time_stamp = (param_dict['sample']['time'] // 1000) / 1000
@@ -37,3 +89,21 @@ def process_event(param_dict):
 
 	# Assume that start time is the time of the first event.
 	start_time = time_stamp if not start_time else start_time
+
+	# Parse the callchain of the current sample into a stack array.
+	if param_dict['callchain']:
+		stack = []
+		for call in param_dict['callchain']:
+			if 'sym' not in call:
+				continue
+			stack.append(call['sym']['name'] + f' (in {call["dso"]})')
+		if len(stack) != 0:
+			stack = stack[::-1]
+			_addThreadSample(pid, tid, thread_name, time_stamp, stack)
+
+	# During perf record if -g is not used, the callchain is not available.
+	# In that case, the symbol and dso are available in the event parameters.
+	else:
+		func = param_dict['symbol'] if 'symbol' in param_dict else '[unknown]'
+		dso = param_dict['dso'] if 'dso' in param_dict else '[unknown]'
+		_addThreadSample(pid, tid, thread_name, time_stamp, [func + f' (in {dso})'])
-- 
2.34.1

Re: [PATCH v3 3/6] scripts: python: thread sample processing to create thread with schemas

[Ian Rogers]

On Mon, Jul 10, 2023 at 4:13 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
>
> The _addThreadSample function is responsible for adding a sample
> to a specific thread. It first checks if the thread exists in
> the thread_map dictionary.
>
> The markers structure defines the schema and data for
> thread markers, including fields such as 'name',
> 'startTime', 'endTime', 'phase', 'category', and 'data'.
>
> The samples structure defines the schema and data for thread
> samples, including fields such as 'stack', 'time', and
> 'responsiveness'.
>
> The frameTable structure defines the schema and data for frame
> information, including fields such as 'location', 'relevantForJS',
> 'innerWindowID', 'implementation', 'optimizations', 'line',
> 'column', 'category', and 'subcategory'.
>
> The purpose of this function is to create a new thread structure
> These structures provide a framework for storing and organizing
> information related to thread markers, samples, frame details,
> and stack information.
>
> The call stack is parsed to include function names and the associated
> DSO, which are requires for creating json schema. Also few libaries
> has been included which will be used in later commit.

nit: s/requires/required.
nit: I think the "Also few..." statement is out-of-date.

> Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> ---
>  .../scripts/python/firefox-gecko-converter.py | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> index 765f1775cee5..0b8a86bdcab1 100644
> --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> @@ -21,6 +21,7 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
>  from perf_trace_context import *
>  from Core import *
>

A comment and type information would be useful here. map is another
word for a dictionary, which is somewhat implied. So the information
here is that this data structure will hold something to do with
threads.  Perhaps say, "a map from TID to a Thread." A better variable
name may then be tid_to_thread_map, but as map is implied you could
do: tid_to_thread: Dict[int, Thread].

> +thread_map = {}
>  start_time = None
>
>  def trace_end():
> @@ -28,6 +29,57 @@ def trace_end():
>
>  def process_event(param_dict):
>         global start_time
> +       global thread_map
> +
> +       def _createThread(name, pid, tid):
> +               markers = {
> +                       'schema': {
> +                               'name': 0,
> +                               'startTime': 1,
> +                               'endTime': 2,
> +                               'phase': 3,
> +                               'category': 4,
> +                               'data': 5,
> +                       },
> +                       'data': [],
> +               }
> +               samples = {
> +                       'schema': {
> +                               'stack': 0,
> +                               'time': 1,
> +                               'responsiveness': 2,
> +                               },
> +                       'data': [],
> +               }
> +               frameTable = {
> +                       'schema': {
> +                               'location': 0,
> +                               'relevantForJS': 1,
> +                               'innerWindowID': 2,
> +                               'implementation': 3,
> +                               'optimizations': 4,
> +                               'line': 5,
> +                               'column': 6,
> +                               'category': 7,
> +                               'subcategory': 8,
> +                       },
> +                       'data': [],
> +               }
> +               stackTable = {
> +                       'schema': {
> +                               'prefix': 0,
> +                               'frame': 1,
> +                       },
> +                       'data': [],
> +               }
> +               stringTable = []

Is there a missing return here?

> +
> +       def _addThreadSample(pid, tid, threadName, time_stamp, stack):
> +               thread = thread_map.get(tid)
> +               if not thread:
> +                       thread = _createThread(threadName, pid, tid)
> +                       thread_map[tid] = thread
> +
>         # Extract relevant information from the event parameters. The event parameters
>         # are in a dictionary:
>         time_stamp = (param_dict['sample']['time'] // 1000) / 1000
> @@ -37,3 +89,21 @@ def process_event(param_dict):
>
>         # Assume that start time is the time of the first event.
>         start_time = time_stamp if not start_time else start_time
> +
> +       # Parse the callchain of the current sample into a stack array.
> +       if param_dict['callchain']:
> +               stack = []
> +               for call in param_dict['callchain']:
> +                       if 'sym' not in call:
> +                               continue
> +                       stack.append(call['sym']['name'] + f' (in {call["dso"]})')

Rather than mix an append and an f-string, just have the f-string ie:
stack.append(f'{call["sym"]["name"]} (in {"call["dso"]})')

> +               if len(stack) != 0:
> +                       stack = stack[::-1]
> +                       _addThreadSample(pid, tid, thread_name, time_stamp, stack)
> +
> +       # During perf record if -g is not used, the callchain is not available.
> +       # In that case, the symbol and dso are available in the event parameters.
> +       else:
> +               func = param_dict['symbol'] if 'symbol' in param_dict else '[unknown]'
> +               dso = param_dict['dso'] if 'dso' in param_dict else '[unknown]'
> +               _addThreadSample(pid, tid, thread_name, time_stamp, [func + f' (in {dso})'])

Similarly:
f'{func} (in {dso})'

Thanks,
Ian

> --
> 2.34.1
>

Re: [PATCH v3 3/6] scripts: python: thread sample processing to create thread with schemas

[Anup Sharma]

On Wed, Jul 12, 2023 at 10:25:50AM -0700, Ian Rogers wrote:
> On Mon, Jul 10, 2023 at 4:13 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
> >
> > The _addThreadSample function is responsible for adding a sample
> > to a specific thread. It first checks if the thread exists in
> > the thread_map dictionary.
> >
> > The markers structure defines the schema and data for
> > thread markers, including fields such as 'name',
> > 'startTime', 'endTime', 'phase', 'category', and 'data'.
> >
> > The samples structure defines the schema and data for thread
> > samples, including fields such as 'stack', 'time', and
> > 'responsiveness'.
> >
> > The frameTable structure defines the schema and data for frame
> > information, including fields such as 'location', 'relevantForJS',
> > 'innerWindowID', 'implementation', 'optimizations', 'line',
> > 'column', 'category', and 'subcategory'.
> >
> > The purpose of this function is to create a new thread structure
> > These structures provide a framework for storing and organizing
> > information related to thread markers, samples, frame details,
> > and stack information.
> >
> > The call stack is parsed to include function names and the associated
> > DSO, which are requires for creating json schema. Also few libaries
> > has been included which will be used in later commit.
> 
> nit: s/requires/required.
> nit: I think the "Also few..." statement is out-of-date.

I apologize. I will ensure to address these in the next version.

> > Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> > ---
> >  .../scripts/python/firefox-gecko-converter.py | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> > index 765f1775cee5..0b8a86bdcab1 100644
> > --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> > +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> > @@ -21,6 +21,7 @@ sys.path.append(os.environ['PERF_EXEC_PATH'] + \
> >  from perf_trace_context import *
> >  from Core import *
> >
> 
> A comment and type information would be useful here. map is another
> word for a dictionary, which is somewhat implied. So the information
> here is that this data structure will hold something to do with
> threads.  Perhaps say, "a map from TID to a Thread." A better variable
> name may then be tid_to_thread_map, but as map is implied you could
> do: tid_to_thread: Dict[int, Thread].

sure, I will rename this variable to tid_to_thread. However in my case
this needs to be kept a global variable, and I am not sure if I can
specify the data type for this variable since it creates a dependency
loop with the Thread class. I can leave it with just a comment
mentioning the type of key and value or I can write the type as
"Dict[int, Any]" which is not very useful.

> > +thread_map = {}
> >  start_time = None
> >
> >  def trace_end():
> > @@ -28,6 +29,57 @@ def trace_end():
> >
> >  def process_event(param_dict):
> >         global start_time
> > +       global thread_map
> > +
> > +       def _createThread(name, pid, tid):
> > +               markers = {
> > +                       'schema': {
> > +                               'name': 0,
> > +                               'startTime': 1,
> > +                               'endTime': 2,
> > +                               'phase': 3,
> > +                               'category': 4,
> > +                               'data': 5,
> > +                       },
> > +                       'data': [],
> > +               }
> > +               samples = {
> > +                       'schema': {
> > +                               'stack': 0,
> > +                               'time': 1,
> > +                               'responsiveness': 2,
> > +                               },
> > +                       'data': [],
> > +               }
> > +               frameTable = {
> > +                       'schema': {
> > +                               'location': 0,
> > +                               'relevantForJS': 1,
> > +                               'innerWindowID': 2,
> > +                               'implementation': 3,
> > +                               'optimizations': 4,
> > +                               'line': 5,
> > +                               'column': 6,
> > +                               'category': 7,
> > +                               'subcategory': 8,
> > +                       },
> > +                       'data': [],
> > +               }
> > +               stackTable = {
> > +                       'schema': {
> > +                               'prefix': 0,
> > +                               'frame': 1,
> > +                       },
> > +                       'data': [],
> > +               }
> > +               stringTable = []
> 
> Is there a missing return here?

No, I am not returning anything here.

> > +
> > +       def _addThreadSample(pid, tid, threadName, time_stamp, stack):
> > +               thread = thread_map.get(tid)
> > +               if not thread:
> > +                       thread = _createThread(threadName, pid, tid)
> > +                       thread_map[tid] = thread
> > +
> >         # Extract relevant information from the event parameters. The event parameters
> >         # are in a dictionary:
> >         time_stamp = (param_dict['sample']['time'] // 1000) / 1000
> > @@ -37,3 +89,21 @@ def process_event(param_dict):
> >
> >         # Assume that start time is the time of the first event.
> >         start_time = time_stamp if not start_time else start_time
> > +
> > +       # Parse the callchain of the current sample into a stack array.
> > +       if param_dict['callchain']:
> > +               stack = []
> > +               for call in param_dict['callchain']:
> > +                       if 'sym' not in call:
> > +                               continue
> > +                       stack.append(call['sym']['name'] + f' (in {call["dso"]})')
> 
> Rather than mix an append and an f-string, just have the f-string ie:
> stack.append(f'{call["sym"]["name"]} (in {"call["dso"]})')

Thanks for this suggestion. I will make this change.

> > +               if len(stack) != 0:
> > +                       stack = stack[::-1]
> > +                       _addThreadSample(pid, tid, thread_name, time_stamp, stack)
> > +
> > +       # During perf record if -g is not used, the callchain is not available.
> > +       # In that case, the symbol and dso are available in the event parameters.
> > +       else:
> > +               func = param_dict['symbol'] if 'symbol' in param_dict else '[unknown]'
> > +               dso = param_dict['dso'] if 'dso' in param_dict else '[unknown]'
> > +               _addThreadSample(pid, tid, thread_name, time_stamp, [func + f' (in {dso})'])
> 
> Similarly:
> f'{func} (in {dso})'

Noted.

> Thanks,
> Ian
> 
> > --
> > 2.34.1
> >

[PATCH v3 4/6] scripts: python: Add trace end processing and JSON output

[Anup Sharma]

Inside the trace end function the final output will be dumped
to standard output in JSON gecko format. Additionally, constants
such as USER_CATEGORY_INDEX, KERNEL_CATEGORY_INDEX, CATEGORIES, and
PRODUCT are defined to provide contextual information.

Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
---
 .../scripts/python/firefox-gecko-converter.py | 34 ++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
index 0b8a86bdcab1..39818a603265 100644
--- a/tools/perf/scripts/python/firefox-gecko-converter.py
+++ b/tools/perf/scripts/python/firefox-gecko-converter.py
@@ -24,8 +24,40 @@ from Core import *
 thread_map = {}
 start_time = None
 
+# Follow Brendan Gregg's Flamegraph convention: orange for kernel and yellow for user
+CATEGORIES = [
+    {'name': 'User', 'color': 'yellow', 'subcategories': ['Other']},
+    {'name': 'Kernel', 'color': 'orange', 'subcategories': ['Other']}
+]
+
+# The product name is used by the profiler UI to show the Operating system and Processor.
+PRODUCT = os.popen('uname -op').read().strip()
+
 def trace_end():
-	pass
+    thread_array = thread_map.values()))
+
+    result = {
+        'meta': {
+            'interval': 1,
+            'processType': 0,
+            'product': PRODUCT,
+            'stackwalk': 1,
+            'debug': 0,
+            'gcpoison': 0,
+            'asyncstack': 1,
+            'startTime': start_time,
+            'shutdownTime': None,
+            'version': 24,
+            'presymbolicated': True,
+            'categories': CATEGORIES,
+            'markerSchema': []
+            },
+        'libs': [],
+        'threads': thread_array,
+        'processes': [],
+        'pausedRanges': []
+    }
+    json.dump(result, sys.stdout, indent=2)
 
 def process_event(param_dict):
 	global start_time
-- 
2.34.1

Re: [PATCH v3 4/6] scripts: python: Add trace end processing and JSON output

[Ian Rogers]

On Mon, Jul 10, 2023 at 4:13 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
>
> Inside the trace end function the final output will be dumped
> to standard output in JSON gecko format. Additionally, constants
> such as USER_CATEGORY_INDEX, KERNEL_CATEGORY_INDEX, CATEGORIES, and
> PRODUCT are defined to provide contextual information.
>
> Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>

Acked-by: Ian Rogers <irogers@google.com>

> ---
>  .../scripts/python/firefox-gecko-converter.py | 34 ++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> index 0b8a86bdcab1..39818a603265 100644
> --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> @@ -24,8 +24,40 @@ from Core import *
>  thread_map = {}
>  start_time = None
>
> +# Follow Brendan Gregg's Flamegraph convention: orange for kernel and yellow for user
> +CATEGORIES = [
> +    {'name': 'User', 'color': 'yellow', 'subcategories': ['Other']},
> +    {'name': 'Kernel', 'color': 'orange', 'subcategories': ['Other']}
> +]

A follow up could be to make these command line options, defaulting to
orange and yellow.

Thanks,
Ian

> +
> +# The product name is used by the profiler UI to show the Operating system and Processor.
> +PRODUCT = os.popen('uname -op').read().strip()
> +
>  def trace_end():
> -       pass
> +    thread_array = thread_map.values()))
> +
> +    result = {
> +        'meta': {
> +            'interval': 1,
> +            'processType': 0,
> +            'product': PRODUCT,
> +            'stackwalk': 1,
> +            'debug': 0,
> +            'gcpoison': 0,
> +            'asyncstack': 1,
> +            'startTime': start_time,
> +            'shutdownTime': None,
> +            'version': 24,
> +            'presymbolicated': True,
> +            'categories': CATEGORIES,
> +            'markerSchema': []
> +            },
> +        'libs': [],
> +        'threads': thread_array,
> +        'processes': [],
> +        'pausedRanges': []
> +    }
> +    json.dump(result, sys.stdout, indent=2)
>
>  def process_event(param_dict):
>         global start_time
> --
> 2.34.1
>

Re: [PATCH v3 4/6] scripts: python: Add trace end processing and JSON output

[Namhyung Kim]

Hi Anup and Ian,

On Wed, Jul 12, 2023 at 10:28 AM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Jul 10, 2023 at 4:13 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
> >
> > Inside the trace end function the final output will be dumped
> > to standard output in JSON gecko format. Additionally, constants
> > such as USER_CATEGORY_INDEX, KERNEL_CATEGORY_INDEX, CATEGORIES, and
> > PRODUCT are defined to provide contextual information.
> >
> > Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
>
> Acked-by: Ian Rogers <irogers@google.com>

I'm ok with this change too but I think it can be squashed to
patch 1/6 as I think it'd make it more self-contained.  Of course
you might change time and thread to have empty values.

>
> > ---
> >  .../scripts/python/firefox-gecko-converter.py | 34 ++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> > index 0b8a86bdcab1..39818a603265 100644
> > --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> > +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> > @@ -24,8 +24,40 @@ from Core import *
> >  thread_map = {}
> >  start_time = None
> >
> > +# Follow Brendan Gregg's Flamegraph convention: orange for kernel and yellow for user
> > +CATEGORIES = [
> > +    {'name': 'User', 'color': 'yellow', 'subcategories': ['Other']},
> > +    {'name': 'Kernel', 'color': 'orange', 'subcategories': ['Other']}
> > +]
>
> A follow up could be to make these command line options, defaulting to
> orange and yellow.

Sounds good.

>
> > +
> > +# The product name is used by the profiler UI to show the Operating system and Processor.
> > +PRODUCT = os.popen('uname -op').read().strip()

I'm not against this but having a command name (or full
command line) of the target process as a title might be better.
But I'm not sure if the python scripting engine exposed the info
(like in perf report --header-only) to the script.

Thanks,
Namhyung


> > +
> >  def trace_end():
> > -       pass
> > +    thread_array = thread_map.values()))
> > +
> > +    result = {
> > +        'meta': {
> > +            'interval': 1,
> > +            'processType': 0,
> > +            'product': PRODUCT,
> > +            'stackwalk': 1,
> > +            'debug': 0,
> > +            'gcpoison': 0,
> > +            'asyncstack': 1,
> > +            'startTime': start_time,
> > +            'shutdownTime': None,
> > +            'version': 24,
> > +            'presymbolicated': True,
> > +            'categories': CATEGORIES,
> > +            'markerSchema': []
> > +            },
> > +        'libs': [],
> > +        'threads': thread_array,
> > +        'processes': [],
> > +        'pausedRanges': []
> > +    }
> > +    json.dump(result, sys.stdout, indent=2)
> >
> >  def process_event(param_dict):
> >         global start_time
> > --
> > 2.34.1
> >

Re: [PATCH v3 4/6] scripts: python: Add trace end processing and JSON output

[Anup Sharma]

On Thu, Jul 13, 2023 at 07:31:36PM -0700, Namhyung Kim wrote:
> Hi Anup and Ian,
> 
> On Wed, Jul 12, 2023 at 10:28 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Mon, Jul 10, 2023 at 4:13 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
> > >
> > > Inside the trace end function the final output will be dumped
> > > to standard output in JSON gecko format. Additionally, constants
> > > such as USER_CATEGORY_INDEX, KERNEL_CATEGORY_INDEX, CATEGORIES, and
> > > PRODUCT are defined to provide contextual information.
> > >
> > > Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> >
> > Acked-by: Ian Rogers <irogers@google.com>
> 
> I'm ok with this change too but I think it can be squashed to
> patch 1/6 as I think it'd make it more self-contained.  Of course
> you might change time and thread to have empty values.
> 
> >
> > > ---
> > >  .../scripts/python/firefox-gecko-converter.py | 34 ++++++++++++++++++-
> > >  1 file changed, 33 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> > > index 0b8a86bdcab1..39818a603265 100644
> > > --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> > > +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> > > @@ -24,8 +24,40 @@ from Core import *
> > >  thread_map = {}
> > >  start_time = None
> > >
> > > +# Follow Brendan Gregg's Flamegraph convention: orange for kernel and yellow for user
> > > +CATEGORIES = [
> > > +    {'name': 'User', 'color': 'yellow', 'subcategories': ['Other']},
> > > +    {'name': 'Kernel', 'color': 'orange', 'subcategories': ['Other']}
> > > +]
> >
> > A follow up could be to make these command line options, defaulting to
> > orange and yellow.
> 
> Sounds good.

Nice Idea, I have added this in the next version of patch.

> >
> > > +
> > > +# The product name is used by the profiler UI to show the Operating system and Processor.
> > > +PRODUCT = os.popen('uname -op').read().strip()
> 
> I'm not against this but having a command name (or full
> command line) of the target process as a title might be better.
> But I'm not sure if the python scripting engine exposed the info
> (like in perf report --header-only) to the script.

I tried to get the command name or any sort of header information but it seems to
be not exposed to the perf-script-python interface. can anyone confirm
if there is any way to get the command name or any header information from the
perf-script-python interface?

> Thanks,
> Namhyung
> 
> 
> > > +
> > >  def trace_end():
> > > -       pass
> > > +    thread_array = thread_map.values()))
> > > +
> > > +    result = {
> > > +        'meta': {
> > > +            'interval': 1,
> > > +            'processType': 0,
> > > +            'product': PRODUCT,
> > > +            'stackwalk': 1,
> > > +            'debug': 0,
> > > +            'gcpoison': 0,
> > > +            'asyncstack': 1,
> > > +            'startTime': start_time,
> > > +            'shutdownTime': None,
> > > +            'version': 24,
> > > +            'presymbolicated': True,
> > > +            'categories': CATEGORIES,
> > > +            'markerSchema': []
> > > +            },
> > > +        'libs': [],
> > > +        'threads': thread_array,
> > > +        'processes': [],
> > > +        'pausedRanges': []
> > > +    }
> > > +    json.dump(result, sys.stdout, indent=2)
> > >
> > >  def process_event(param_dict):
> > >         global start_time
> > > --
> > > 2.34.1
> > >

[PATCH v3 5/6] scripts: python: Implement add sample function and return finish

[Anup Sharma]

The addSample function appends a new entry to the 'samples' data structure.

The finish function generates a dictionary containing various profile
information such as 'tid', 'pid', 'name', 'markers', 'samples',
'frameTable', 'stackTable', 'stringTable', 'registerTime',
'unregisterTime', and 'processType'.

Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
---
 .../scripts/python/firefox-gecko-converter.py | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
index 39818a603265..6c934de1f608 100644
--- a/tools/perf/scripts/python/firefox-gecko-converter.py
+++ b/tools/perf/scripts/python/firefox-gecko-converter.py
@@ -106,11 +106,36 @@ def process_event(param_dict):
 		}
 		stringTable = []
 
+		def addSample(threadName, stackArray, time):
+			responsiveness = 0
+			samples['data'].append([stack, time, responsiveness])
+
+		def finish():
+			return {
+				"tid": tid,
+				"pid": pid,
+				"name": name,
+				"markers": markers,
+				"samples": samples,
+				"frameTable": frameTable,
+				"stackTable": stackTable,
+				"stringTable": stringTable,
+				"registerTime": 0,
+				"unregisterTime": None,
+				"processType": 'default'
+			}
+
+		return {
+			"addSample": addSample,
+			"finish": finish
+		}
+
 	def _addThreadSample(pid, tid, threadName, time_stamp, stack):
 		thread = thread_map.get(tid)
 		if not thread:
 			thread = _createThread(threadName, pid, tid)
 			thread_map[tid] = thread
+		thread['addSample'](threadName, stack, time_stamp)
 
 	# Extract relevant information from the event parameters. The event parameters
 	# are in a dictionary:
-- 
2.34.1

Re: [PATCH v3 5/6] scripts: python: Implement add sample function and return finish

[Ian Rogers]

On Mon, Jul 10, 2023 at 4:14 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
>
> The addSample function appends a new entry to the 'samples' data structure.
>
> The finish function generates a dictionary containing various profile
> information such as 'tid', 'pid', 'name', 'markers', 'samples',
> 'frameTable', 'stackTable', 'stringTable', 'registerTime',
> 'unregisterTime', and 'processType'.
>
> Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> ---
>  .../scripts/python/firefox-gecko-converter.py | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> index 39818a603265..6c934de1f608 100644
> --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> @@ -106,11 +106,36 @@ def process_event(param_dict):
>                 }
>                 stringTable = []
>
> +               def addSample(threadName, stackArray, time):

I think these aren't following general naming conventions:
https://peps.python.org/pep-0008/#function-and-variable-names
So use thread_name, stack_array.

> +                       responsiveness = 0
> +                       samples['data'].append([stack, time, responsiveness])
> +
> +               def finish():
> +                       return {
> +                               "tid": tid,
> +                               "pid": pid,
> +                               "name": name,
> +                               "markers": markers,
> +                               "samples": samples,
> +                               "frameTable": frameTable,
> +                               "stackTable": stackTable,
> +                               "stringTable": stringTable,
> +                               "registerTime": 0,
> +                               "unregisterTime": None,
> +                               "processType": 'default'
> +                       }
> +
> +               return {
> +                       "addSample": addSample,
> +                       "finish": finish
> +               }

I think the use of a dictionary here isn't idiomatic. Rather than use
a dictionary I think you can make a class Thread, then have functions
passed self called addSample and finish. So:

class Thread:
  def addSample(self, thread_name: str, stack_array: list[...], time: int):
     responsiveness = 0
     self.samples['data'] ...
...
thread.addSample(threadName, stack, time_stamp)

Should samples be its own class here?

Thanks,
Ian

> +
>         def _addThreadSample(pid, tid, threadName, time_stamp, stack):
>                 thread = thread_map.get(tid)
>                 if not thread:
>                         thread = _createThread(threadName, pid, tid)
>                         thread_map[tid] = thread
> +               thread['addSample'](threadName, stack, time_stamp)
>
>         # Extract relevant information from the event parameters. The event parameters
>         # are in a dictionary:
> --
> 2.34.1
>

Re: [PATCH v3 5/6] scripts: python: Implement add sample function and return finish

[Anup Sharma]

On Wed, Jul 12, 2023 at 10:35:16AM -0700, Ian Rogers wrote:
> On Mon, Jul 10, 2023 at 4:14 PM Anup Sharma <anupnewsmail@gmail.com> wrote:
> >
> > The addSample function appends a new entry to the 'samples' data structure.
> >
> > The finish function generates a dictionary containing various profile
> > information such as 'tid', 'pid', 'name', 'markers', 'samples',
> > 'frameTable', 'stackTable', 'stringTable', 'registerTime',
> > 'unregisterTime', and 'processType'.
> >
> > Signed-off-by: Anup Sharma <anupnewsmail@gmail.com>
> > ---
> >  .../scripts/python/firefox-gecko-converter.py | 25 +++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/tools/perf/scripts/python/firefox-gecko-converter.py b/tools/perf/scripts/python/firefox-gecko-converter.py
> > index 39818a603265..6c934de1f608 100644
> > --- a/tools/perf/scripts/python/firefox-gecko-converter.py
> > +++ b/tools/perf/scripts/python/firefox-gecko-converter.py
> > @@ -106,11 +106,36 @@ def process_event(param_dict):
> >                 }
> >                 stringTable = []
> >
> > +               def addSample(threadName, stackArray, time):
> 
> I think these aren't following general naming conventions:
> https://peps.python.org/pep-0008/#function-and-variable-names
> So use thread_name, stack_array.

Noted. Will fix in v4.

> > +                       responsiveness = 0
> > +                       samples['data'].append([stack, time, responsiveness])
> > +
> > +               def finish():
> > +                       return {
> > +                               "tid": tid,
> > +                               "pid": pid,
> > +                               "name": name,
> > +                               "markers": markers,
> > +                               "samples": samples,
> > +                               "frameTable": frameTable,
> > +                               "stackTable": stackTable,
> > +                               "stringTable": stringTable,
> > +                               "registerTime": 0,
> > +                               "unregisterTime": None,
> > +                               "processType": 'default'
> > +                       }
> > +
> > +               return {
> > +                       "addSample": addSample,
> > +                       "finish": finish
> > +               }
> 
> I think the use of a dictionary here isn't idiomatic. Rather than use
> a dictionary I think you can make a class Thread, then have functions
> passed self called addSample and finish. So:

agreed.

> class Thread:
>   def addSample(self, thread_name: str, stack_array: list[...], time: int):
>      responsiveness = 0
>      self.samples['data'] ...
> ...
> thread.addSample(threadName, stack, time_stamp)
>
> Should samples be its own class here?

I have changed the code to use object oriented approach by taking
reference from simpleperf. I will make class Thread and Sample.

> Thanks,
> Ian
> 
> > +
> >         def _addThreadSample(pid, tid, threadName, time_stamp, stack):
> >                 thread = thread_map.get(tid)
> >                 if not thread:
> >                         thread = _createThread(threadName, pid, tid)
> >                         thread_map[tid] = thread
> > +               thread['addSample'](threadName, stack, time_stamp)
> >
> >         # Extract relevant information from the event parameters. The event parameters
> >         # are in a dictionary:
> > --
> > 2.34.1
> >

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

[Anup Sharma]

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@gmail.com>
---
 .../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()))
+
+# Parse the callchain of the current sample into a stack array.
+    for thread in thread_array:
+        key = thread['samples']['schema']['time']
+        thread['samples']['data'].sort(key=lambda data : float(data[key]))
 
     result = {
         'meta': {
@@ -106,7 +113,55 @@ def process_event(param_dict):
 		}
 		stringTable = []
 
+		stackMap = dict()
+		def get_or_create_stack(frame, prefix):
+			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):
+			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
+				implementation = None
+				optimizations = None
+				line = None
+				relevantForJS = False
+				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
+			stack = reduce(lambda prefix, stackFrame: get_or_create_stack
+					(get_or_create_frame(stackFrame), prefix), stackArray, None)
 			responsiveness = 0
 			samples['data'].append([stack, time, responsiveness])
 
-- 
2.34.1

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@gmail.com> 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@gmail.com>
> ---
>  .../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
>

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

[Anup Sharma]

On Wed, Jul 12, 2023 at 10:44:21AM -0700, Ian Rogers wrote:
> On Mon, Jul 10, 2023 at 4:15 PM Anup Sharma <anupnewsmail@gmail.com> 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@gmail.com>
> > ---
> >  .../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()

I have made the changes. Thanks for the suggestion.

> > +
> > +# 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.

Thanks, I am planning to add hyper-link as a the comment. Like this:
# https://github.com/firefox-devtools/profiler/blob/53970305b51b9b472e26d7457fee1d66cd4e2737/src/types/gecko-profile.js#L216
However it is going to exceed 100 characters limit. If I wrap it
around, it will look ugly. Any suggestions?

> 
> > +        thread['samples']['data'].sort(key=lambda data : float(data[key]))
> 
> Perhaps there is a more intention revealing name than "data".

Noted. I will change it.

> >
> >      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.

I am using more descriptive names now. I have also added a comment like
this to the function:
"""Gets a matching stack, or saves the new stack. Returns a Stack ID."""
Will be reflected in v4.

> > +                       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".

I have made the changes. Thanks for the suggestion.

> > +                       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.

I am trying to find if I can use cpu_mode instead of this as suggested by
Namhyung. If not, I will add a helper function in the later version.

> > +                               implementation = None
> > +                               optimizations = None
> > +                               line = None
> > +                               relevantForJS = False
> 
> Some comments on these variables would be useful, perhaps just use
> named arguments below.

Okay, this variable comes from Gecko format, now adding link might
make it understandable.

> > +                               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.

Noted.

> 
> > +                       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
> >

Re: [PATCH v3 0/6] Add support for Firefox's gecko profile format

[Anup Sharma]

On Tue, Jul 11, 2023 at 04:38:06AM +0530, Anup Sharma wrote:
> This patch series adds support for Firefox's gecko profile format.
> The format is documented here [1].
> 
> I have incorporated several changes based on feedback from the
> previous version of the patch. However, there are still a few
> remaining comments that I am currently addressing.
> 
> Additionally, I am still in the process of learning how to send
> patches in a series, so there may be some shortcomings in this
> particular series as well.
> 
> changes from v2:
> - renamed mod to func
> - removed unnessecary imports print_function
> - removed shebang python env declaration
> - changed _createtread to _createThread
> - Commits in better order.

Hello everyone, I wanted to express my gratitude for your support.
I recently received an email from GSoC admin informing me that I
have successfully passed my midterm evaluation. I am thrilled about
this news!. Ian, could you please provide some insights on the commit
arrangement in v3 compared to v2? I noticed in v3, on 1/6 you have
mentioned regarding the commit arrangement. I want to make sure I
haven't overlooked. Currently, I'm focusing on developing the next
version of patches, so having this information would be extremely
beneficial to me. Thank you in advance!

> [1] https://github.com/firefox-devtools/profiler/blob/main/docs-developer/gecko-profile-format.md
> 
> Anup Sharma (6):
>   scripts: python: Add initial script file with imports.
>   scripts: python: Extact necessary information from process event
>   scripts: python: thread sample processing to create thread with
>     schemas
>   scripts: python: Add trace end processing and JSON output
>   scripts: python: Implement add sample function and return finish
>   scripts: python: implement get or create frame and stack function
> 
>  .../scripts/python/firefox-gecko-converter.py | 221 ++++++++++++++++++
>  1 file changed, 221 insertions(+)
>  create mode 100644 tools/perf/scripts/python/firefox-gecko-converter.py
> 
> -- 
> 2.34.1
>