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 <[email protected]>
---
.../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
On Mon, Jul 10, 2023 at 4:15 PM Anup Sharma <[email protected]> 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 <[email protected]>
> ---
> .../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
>
On Wed, Jul 12, 2023 at 10:44:21AM -0700, Ian Rogers wrote:
> On Mon, Jul 10, 2023 at 4:15 PM Anup Sharma <[email protected]> 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 <[email protected]>
> > ---
> > .../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
> >