Skip to content

Branch misprediction rule #262

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

salvatoredipietro
Copy link
Contributor

If more than 20% difference between two runs, it should trigger the rule.

Tested with local APerf profiles. Here the output:

Branch  Misprediction is 71% different between  'aperf_20250410T181733_PG_old_stable200_m8g24xlarge' and  'aperf_20250410T170035_PG_old_stable300_m8g24xlarge'. This can  significantly impact performance since every time the CPU predicts  incorrectly it has to flush the current set of instructions it was  working on and start over by fetching new instructions from the correct  place

@salvatoredipietro salvatoredipietro requested a review from a team as a code owner April 22, 2025 20:30
Copy link
Contributor

@geoffreyblake geoffreyblake left a comment

Choose a reason for hiding this comment

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

See comments in code.

yield new Finding(
`Branch Misprediction is ${diff}% different between '${ruleOpts.base_run}' and '${ruleOpts.this_run}'. ` +
`This can significantly impact performance since every time the CPU predicts incorrectly it has to flush ` +
`the current set of instructions it was working on and start over by fetching new instructions from the correct place.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably too verbose. Should limit it to a single line if possible I think. For more detail we may want to link to other AWS documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but I would like that at same time the user can understand what these metrics are about.
It would be nice to have a link to which we can refer to so that we can keep it here short but user can get more details if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree such documentation should exist somewhere, but I don't agree that it should be on the "findings" page which is supposed to be a summary of what was flagged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall there is an optional extra argument to the Finding intended for this sort of extra detail (but I don't recall how / where it is displayed).

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