Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Refactor PVF preparation memory stats #6693

Merged
merged 7 commits into from
Feb 14, 2023

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Feb 9, 2023

PULL REQUEST

Overview

The original purpose of this change was to gate metrics that are unsupported by some systems, behind conditional compilation directives (#[cfg]); see #6675 (comment).

Then I started doing some random cleanups and simplifications and got a bit carried away. 🙈 The code should be overall tidier than before.

Changes

  • Don't register unsupported metrics (e.g. max_rss on non-Linux systems)
  • Introduce PrepareStats struct as an abstraction over the Ok values of PrepareResult. It is cleaner, and can be easily modified in the future.
  • Other small changes

The original purpose of this change was to gate metrics that are unsupported by
some systems behind conditional compilation directives (#[cfg]); see
#6675 (comment).

Then I started doing some random cleanups and simplifications and got a bit
carried away. 🙈 The code should be overall tidier than before.

Changes:
- Don't register unsupported metrics (e.g. `max_rss` on non-Linux systems)
- Introduce `PrepareStats` struct as an abstraction over the `Ok` values of
  `PrepareResult`. It is cleaner, and can be easily modified in the future.
- Other small changes
@mrcnski mrcnski added I8-refactor Code needs refactoring. A0-please_review Pull request needs code review. I4-annoyance Code behaves within expectations, however this “expected behaviour” itself is at issue. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance. labels Feb 9, 2023
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looks good from a quick glance.

@@ -247,4 +225,26 @@ mod getrusage {
}
Ok(result)
}

/// Gets the `ru_maxrss` for the current thread if the OS supports `getrusage`. Otherwise, just
/// returns `None`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems outdated extract_max_rss_stat returns an option, get_max_rss_thread returns io::Result

@mrcnski
Copy link
Contributor Author

mrcnski commented Feb 14, 2023

bot merge

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. I4-annoyance Code behaves within expectations, however this “expected behaviour” itself is at issue. I8-refactor Code needs refactoring. T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants