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

Add support for selecting CPU(s) #203

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Add support for selecting CPU(s) #203

merged 1 commit into from
Jul 12, 2024

Conversation

janaknat
Copy link
Contributor

@janaknat janaknat commented Jul 1, 2024

This will allow the user to select the CPU(s) for all the graphs in CPU Utilization, Perf Stat and Interrupt tabs.

icocpus.tar.gz

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@janaknat janaknat requested a review from a team as a code owner July 1, 2024 21:23
@wash-amzn
Copy link
Contributor

When "All" isn't selected, aggregate graphs (PMU Stats tab) should also be disabled.

name: `CPU ${cpu}`,
x: x_time,
y: y_data,
type: 'scatter',
};
if (interrupt_cpu_list.get(run).indexOf(cpu.toString()) == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Up in cpu_utilization you toString the cpu variable earlier.

If you're going to have repeated code like this (which ideally would be avoided), at least keep it consistent.

But really, try to find ways to extract out things like this which look identical, or almost identical, across multiple data types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is due to the different way data is packaged. Also, we wanted to avoid checking for 'Aggregate' in multiple places.

grid-column: 1;
}

.configure {
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches a class, but you've named the button configure in index.html. That's why this background image isn't showing up. When you fix that, you'll see the issue with its position.

<meta name="viewport" content="width=device-width, initial-scale=1">
<title>APerf</title>
</head>
<body>
<div class="tab">
<div class="aperf">APerf</div>
<button class="tablinks" name="configure"><div class="icon-text">Configure</div><div class="configure"/></button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to make this look different? Perhaps when the settings icon is working that will help, but I'm thinking of making it obviously not a peer to the other tabs which are data types.

@@ -199,6 +199,7 @@ pub fn report(report: &Report) -> Result<()> {

info!("Creating APerf report...");
let ico = include_bytes!("html_files/favicon.ico");
let configure = include_bytes!("html_files/configure.png");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I see where it's used now. Multiple variables with similar names.

This will allow the user to select the CPU(s) for all the graphs in CPU
Utilization, Perf Stat and Interrupt tabs.
@janaknat janaknat merged commit 477d726 into main Jul 12, 2024
6 checks passed
@janaknat janaknat deleted the cpus-configure branch July 12, 2024 16:23
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.

2 participants