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

Make the Y-axis range match for keys across runs #82

Merged
merged 7 commits into from
Sep 25, 2023
Merged

Conversation

janaknat
Copy link
Contributor

This helps to make it easy to compare the graphs without the need to adjust the y-axis range manually.

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

kernel-build.tar.gz

@wash-amzn
Copy link
Contributor

In the provided report, the left-side menu isn't staying pinned, but that change was already merged ...

@wash-amzn
Copy link
Contributor

Now we need to get matched graphs aligned horizonally.

And default to hiding the pairs of graphs when they are all-zeros.

@janaknat
Copy link
Contributor Author

Now we need to get matched graphs aligned horizonally.

And default to hiding the pairs of graphs when they are all-zeros.

That is the next PR. The graph will be empty with no points if that key did not exist for a particular run.

@janaknat
Copy link
Contributor Author

In the provided report, the left-side menu isn't staying pinned, but that change was already merged ...

That PR got merged after this branch was made.

@@ -163,17 +163,27 @@ pub struct MemEntry {
pub value: u64,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
struct EndMemValues {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these new structs got named starting with End and others did not.

If you're going to continue making changes across all data types, at least try and keep the naming consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Used End for all the structs.

Comment on lines +181 to +178
if value > self.limits.high {
self.limits.high = value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How does an initial value of high get set?

Does this only work because something like value > undefined always winds up being true?

If there's only a single data-point, does high remaining undefined crash the report?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The data structure is initialized with 0 for low and high. After that, when update_limits is called the low and high are set.

Copy link
Contributor

Choose a reason for hiding this comment

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

So you're crossing your fingers here that we never have a negative value?

Set both to the first value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We technically don't support negative numbers. When we do get it, we cast it to uint and use that. See GraphLimitType which has 3 variants, uint64, int64 and f64. int64 casts the value to u64 and re-uses uint64's implementation.

@janaknat janaknat merged commit cf8af36 into main Sep 25, 2023
4 checks passed
@janaknat janaknat deleted the yaxis-update branch September 25, 2023 17:45
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