Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perf converter script to use JSON input on default #226

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

weilinwa
Copy link
Contributor

This is the initial code version to use JSON input for metrics. I'm creating this PR to collect inputs about the code change, for our transition from using spreadsheet to JSON. Please leave your comments and suggestions in this PR. Thank you very much!

I'm hoping to make this transition in two phases. In this first phase, the major goal is to keep both JSON and spreadsheet input work and make the output as close as possible.
In the second phase (after we achieving an agreement on the content and format of final output files), I will work on optimizing the code.

Changes:

  1. On default, the script would use the metric JSON files for each platform. To use the spreadsheet as before, please add option --csv(/-c).

  2. There are a lot of changes in the final metric output files. Some of them are caused by the change of input. There also are changes that we added in intentionally. An example is the new tags added in MetricGroup field, like Slots and Clocks. These are content from the CountDomain column.

    The following is a list of these tags: "Clocks, Clocks_Calculated, Clocks_Estimated, Clocks_Latency, Core_Clocks, Core_Execution, Core_Metric, Core_Utilization, Count, Fraction, GB/sec, Inst_Metric, MB/sec, Metric, NanoSeconds, Scaled_Slots, Slots, Slots_Estimated, Stalls, SystemMetric, Uops"

Copy link
Contributor

@captain5050 captain5050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: There's a typo in the commit message, extract rather than extrac

@edwarddavidbaker
Copy link
Contributor

@captain5050 Does it make sense to continue running create_perf_json.py as a basic GitHub action check? We can temporarily disable it until the necessary components are in place.

scripts/create_perf_json.py Outdated Show resolved Hide resolved
scripts/create_perf_json.py Show resolved Hide resolved
scripts/create_perf_json.py Outdated Show resolved Hide resolved
@captain5050
Copy link
Contributor

@captain5050 Does it make sense to continue running create_perf_json.py as a basic GitHub action check? We can temporarily disable it until the necessary components are in place.

I'd defer to Weilin. She wanted me to look at these changes as a heads up, hopefully the final PR will not break the action. The action is nice in case an event update breaks the converter script

Copy link
Contributor

@captain5050 captain5050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the period will be inserted by perf list, but if it is useful to do things this way to keep the metrics in sync across tools, I'm easy.

@weilinwa
Copy link
Contributor Author

weilinwa commented Oct 1, 2024

I think the period will be inserted by perf list, but if it is useful to do things this way to keep the metrics in sync across tools, I'm easy.

Hi @captain5050, do you mean the period in brief description?

@weilinwa
Copy link
Contributor Author

weilinwa commented Oct 1, 2024

@captain5050 Does it make sense to continue running create_perf_json.py as a basic GitHub action check? We can temporarily disable it until the necessary components are in place.

I'd defer to Weilin. She wanted me to look at these changes as a heads up, hopefully the final PR will not break the action. The action is nice in case an event update breaks the converter script

Hi @edwarddavidbaker, the new script from this PR will need to work with updated JSON input from Valkyrie. Currently, we have upload JSON input of the two platforms included in the PR as examples here. The rest of the platforms might trigger missing event or incorrect event errors like what we see in the check.
I think the safe way to progress is keeping this check on and uploading JSON input of other platforms into this same PR until we resolve all the errors.
Please let me know your thoughts. Thanks!

@weilinwa weilinwa force-pushed the perf_converter_remove_csv_dep branch from 6a7de3f to c6f0c2f Compare October 2, 2024 14:18
@weilinwa
Copy link
Contributor Author

weilinwa commented Oct 2, 2024

nit: There's a typo in the commit message, extract rather than extrac

The commit message is updated. Thanks!

@captain5050
Copy link
Contributor

I think the period will be inserted by perf list, but if it is useful to do things this way to keep the metrics in sync across tools, I'm easy.

Hi @captain5050, do you mean the period in brief description?

Yeah, the introduction of the "." is causing a lot of diffs and perf-list will add the period when necessary in places like:
https://github.com/googleprodkernel/linux-perf/blob/google_tools_master/tools/perf/builtin-list.c#L176

scripts/create_perf_json.py Outdated Show resolved Hide resolved
scripts/create_perf_json.py Outdated Show resolved Hide resolved
scripts/create_perf_json.py Outdated Show resolved Hide resolved
scripts/create_perf_json.py Outdated Show resolved Hide resolved
scripts/create_perf_json.py Outdated Show resolved Hide resolved
scripts/create_perf_json.py Outdated Show resolved Hide resolved
@weilinwa weilinwa force-pushed the perf_converter_remove_csv_dep branch from 6199569 to ae7891a Compare October 14, 2024 12:30
Copy link
Contributor

@edwarddavidbaker edwarddavidbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with Weilin. The script is currently failing on event uops_retired.retire_tma_info_thread_slots used in Broadwell metrics JSON files. This will be fixed in an upcoming revision of the metric files.

public/perfmon/BDW/metrics$ grep -r 'uops_retired.retire_tma' ./
./broadwell_metrics.json:      "BaseFormula": "( uops_issued.any - ( uops_retired.retire_tma_info_thread_slots ) + ( 4 ) * ( ( int_misc.recovery_cycles_any / 2 ) if smt_on else int_misc.recovery_cycles ) ) / tma_info_thread_slots",
./broadwell_metrics.json:      "BaseFormula": "( uops_retired.retire_tma_info_thread_slots ) / tma_info_thread_slots",
./broadwell_metrics.json:      "BaseFormula": "( ( uops_retired.retire_tma_info_thread_slots ) / uops_issued.any ) * idq.ms_uops / tma_info_thread_slots",
./perf/broadwell_metrics_perf.json:        "MetricExpr": "( UOPS_ISSUED.ANY - ( uops_retired.retire_tma_info_thread_slots ) + ( 4 ) * ( ( INT_MISC.RECOVERY_CYCLES_ANY / 2 ) if #SMT_on else INT_MISC.RECOVERY_CYCLES ) ) / tma_info_thread_slots",
./perf/broadwell_metrics_perf.json:        "MetricExpr": "( uops_retired.retire_tma_info_thread_slots ) / tma_info_thread_slots",
./perf/broadwell_metrics_perf.json:        "MetricExpr": "( ( uops_retired.retire_tma_info_thread_slots ) / UOPS_ISSUED.ANY ) * IDQ.MS_UOPS / tma_info_thread_slots",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants