Skip to content

Conversation

@liqiqiii
Copy link
Contributor

@liqiqiii liqiqiii commented Jul 9, 2025

Format output xml and align GUID format for fpdt_parser.
Apply minidom to format the output xml of the fpdt_parser.py to improve readability.
The script was not adding entries to FBDT table so the output is not a proper tree structure.
Before it was:

|- fbpt_header
|- fbpt_record
|- fbpt_record

Now it is:

|- fbpt_header
|   |- fbpt_record
|   |- fbpt_record

it can be properly consumed by perf_report_generator.py to generate perf report. Before this change there will be 0 entries in the report because the input structure doesn't match what the script needs.

Update the GUID format for different entries to make sure the output GUID format stay the same and can be recognized by the perf_report_generator.py.
Add xml formatter to make sure the output xml more readable.

Examples for GUID format change:

Before this fix:
For DynamicStringEvent: 
            `<GUID Value="23C9322F-2AF2-476A-BC4C-26BC-8826-6C71"/>`
For DualGuidStringEvent: 
            `<GUID1 Value="6D33944A-EC75-4855-A54D-809C75241F6C"/>`
            `<GUID2 Value="7B94C75C-36A4-4AA4-A1DF-14BC9A049AE4"/>`

After this fix:
All event guids will be in the same format of 
            `<GUID1 Value="6D33944A-EC75-4855-A54D-809C75241F6C"/>`

With this change the perf_report_generator.py can successfully parse these GUIDs and map to the right module name.

  • Breaking change?
    Detail of the changes can be seen above. The structure and content of the output xml for fpdt_parser.py changed.

  • Integration instruction:
    Use fpdt_parser.py first and use the output xml file as the input for the perf_report_generator.py to get perf report.

@Javagedes Javagedes requested a review from makubacki July 10, 2025 15:21
@Javagedes Javagedes added this to the v0.30.0 milestone Jul 10, 2025
@Javagedes
Copy link
Contributor

@makubacki can you take a look at this? It looks fine to me, but I'm unsure. Marking this as a breaking change as it changes the returned data.

Copy link
Contributor

@Javagedes Javagedes left a comment

Choose a reason for hiding this comment

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

At a minimum, it looks like you need to run the ruff formatter to be compliant with the CI

@liqiqiii liqiqiii requested a review from Javagedes July 10, 2025 23:51
Copy link
Contributor

@Javagedes Javagedes left a comment

Choose a reason for hiding this comment

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

This is quite the breaking change. Can you please add additional description to this PR regarding that?

Specifically, I think the breaking changes are:

  1. The Guid format changing
  2. The overall format of the xml appears to be changing:
|- fbpt_header
|- fbpt_record
|- fbpt_record

to

|- fbpt_header
|   |- fbpt_record
|   |- fbpt_record

Specifically, please provide examples of the changes, and integration instructions for anyone that uses this tool.

You also need to run the ruff formatter to be compliant with CI :)

@makubacki
Copy link
Member

@liqiqiii, the changes look fine, I'll approve after Joey's comments are addressed.

@liqiqiii
Copy link
Contributor Author

This is quite the breaking change. Can you please add additional description to this PR regarding that?

Specifically, I think the breaking changes are:

  1. The Guid format changing
  2. The overall format of the xml appears to be changing:
|- fbpt_header
|- fbpt_record
|- fbpt_record

to

|- fbpt_header
|   |- fbpt_record
|   |- fbpt_record

Specifically, please provide examples of the changes, and integration instructions for anyone that uses this tool.

You also need to run the ruff formatter to be compliant with CI :)

Hi Joey, I have updated the PR description.
I didn't think of it as a breaking change at the beginning because to me it looks more like a bug fix, before its output can't be processed properly by the perf_report_generator.py.
I agree with you as I have changed the output format so it's indeed a breaking change. Thanks for the reminder.
I have run the ruff and fixed the CI gates.
Thank you!

@liqiqiii liqiqiii requested a review from Javagedes July 11, 2025 21:48
@Javagedes Javagedes added the enhancement New feature or request label Jul 14, 2025
@Javagedes Javagedes added the bug Something isn't working label Jul 14, 2025
@Javagedes
Copy link
Contributor

@liqiqiii bump for this PR so we can get CI working and this merged in

@codecov
Copy link

codecov bot commented Jul 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.46%. Comparing base (bf643b0) to head (3d4ce8e).
⚠️ Report is 168 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1006      +/-   ##
==========================================
+ Coverage   78.75%   79.46%   +0.71%     
==========================================
  Files          49       44       -5     
  Lines        4909     6273    +1364     
==========================================
+ Hits         3866     4985    +1119     
- Misses       1043     1288     +245     
Files with missing lines Coverage Δ
edk2toolext/perf/fpdt_parser.py 90.77% <100.00%> (ø)

... and 49 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Javagedes Javagedes merged commit 1a5fa06 into tianocore:master Aug 4, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants