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.

Sign up for GitHub

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

Jump to bottom

Refactor Execute Timings to use an enum-indexed array structure #23085

Merged
merged 1 commit into from Feb 22, 2022
Merged

Refactor Execute Timings to use an enum-indexed array structure #23085

merged 1 commit into from Feb 22, 2022

Conversation

antidmg
Copy link
Contributor

@antidmg antidmg commented Feb 11, 2022 •
edited by t-nelson

Problem

The ExecuteTimings type was getting a bit complex and clunky to use. Context here

This is my first attempt at a contribution, so I'm new to the codebase (and to Rust). I saw this issue hasn't been active for over a month, so I decided to take a stab at it.

Any feedback is appreciated, thank you! (@t-nelson or anyone else)

Summary of Changes

  • Replace all of the previous metrics fields in ExecuteTimings with an enum-indexed array.
  • Add an in-place method for saturating_add

A few questions:

  1. Is there a better way to implement the std::ops::Index?
  2. Can we get rid of the hardcoded 7 for the Metrics type?

Fixes #22325
Consider refactoring ExecuteTimings type

@mergify mergify bot added the community Community contribution label Feb 11, 2022
@mergify mergify bot requested a review from a team February 11, 2022 16:58
t-nelson
t-nelson reviewed Feb 11, 2022
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for taking this on ❤️

program-runtime/src/timings.rs Outdated Show resolved Hide resolved
program-runtime/src/timings.rs Outdated Show resolved Hide resolved
program-runtime/src/timings.rs Outdated Show resolved Hide resolved
program-runtime/src/timings.rs Outdated Show resolved Hide resolved
@antidmg antidmg requested review from t-nelson and removed request for a team February 13, 2022 02:38
@codecov
Copy link

codecov bot commented Feb 17, 2022 •
edited

Codecov Report

Merging #23085 (f321fa7) into master (d5dec98) will increase coverage by 0.0%.
The diff coverage is 63.7%.

@@           Coverage Diff           @@
##           master   #23085   +/-   ##
=======================================
  Coverage    81.2%    81.2%           
=======================================
  Files         564      564           
  Lines      153429   153454   +25     
=======================================
+ Hits       124649   124707   +58     
+ Misses      28780    28747   -33

t-nelson
t-nelson previously approved these changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks again for picking this up!

@t-nelson
Copy link
Contributor

@jackcmay @Lichtso I think you two originally solicited this change. Mind taking a peek?

@antidmg
Copy link
Contributor Author

antidmg commented Feb 18, 2022 •
edited

Looks great! Thanks again for picking this up!

Of course, it was fun! Please let me know if there are other issues like this lying around 😄

jackcmay
jackcmay reviewed Feb 18, 2022
View reviewed changes
program-runtime/src/timings.rs Outdated

pub fn saturating_add_in_place(&mut self, timing_type: ExecuteTimingType, value_to_add: u64) {
let idx = timing_type as usize;
let res = self.metrics[idx].saturating_add(value_to_add);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though I see its safe here, direct indexing makes me nervous from a panic standpoint. Could use get_mut here and ignore if index is out of bounds. Also a debug assert on the index might be helpful

jackcmay
jackcmay reviewed Feb 18, 2022
View reviewed changes
core/src/progress_map.rs Outdated
("store_us", self.execute_timings.store_us, i64),
(
"check_us",
self.execute_timings.metrics[ExecuteTimingType::CheckUs],
Copy link
Contributor

Choose a reason for hiding this comment

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

These direct indexes worry me to, safe here, yes, safe in the future, ?

Copy link
Contributor Author

@antidmg antidmg Feb 20, 2022 •
edited

Choose a reason for hiding this comment

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

That's a good point, thanks for pointing this out! I tried wrapping the array in a newtype to prevent users from calling timings.metrics[<bad_usize>]. Let me know if there's a better alternative.

@jackcmay
Copy link
Contributor

Thanks for the contribution, looking good, couple of nits

@mergify mergify bot dismissed t-nelson’s stale review February 20, 2022 18:20

Pull request has been modified.

@jackcmay jackcmay merged commit 20d031e into solana-labs:master Feb 22, 2022
@antidmg antidmg deleted the refactor-execute-timings branch February 23, 2022 04:45
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 4, 2022
@jeffwashington
Refactor ExecuteTimings w/ enum-indexed array (solana-labs#23085)
4ac25a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackcmay jackcmay jackcmay approved these changes

@t-nelson t-nelson t-nelson left review comments

@Lichtso Lichtso Awaiting requested review from Lichtso

Assignees
No one assigned
Labels
community Community contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider refactoring ExecuteTimings type
3 participants
@antidmg @t-nelson @jackcmay