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 execute metrics #22296

Merged
merged 6 commits into from Jan 6, 2022
Merged

Add execute metrics #22296

merged 6 commits into from Jan 6, 2022

Conversation

t-nelson
Copy link
Contributor

@t-nelson t-nelson commented Jan 5, 2022

Problem

Execution is occasionally slow but cause is not captured in current metrics

Summary of Changes

Add more timings

Republish of #22043, since I apparently broke Github over there

@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #22296 (d7c54f6) into master (d922065) will increase coverage by 0.0%.
The diff coverage is 88.7%.

@@           Coverage Diff            @@
##           master   #22296    +/-   ##
========================================
  Coverage    81.0%    81.0%            
========================================
  Files         551      551            
  Lines      149617   149775   +158     
========================================
+ Hits       121301   121454   +153     
- Misses      28316    28321     +5     

@Lichtso
Copy link
Contributor

Lichtso commented Jan 5, 2022

Two things:

  • Have you run and measured these against MB? Because some of the metrics might measure so little that the measurement itself is more expensive.
  • There is a lot of b = b.saturating_add(a); which is not only hard to read but also error prone as it can lead to copy pasta like d = b.saturating_add(a);. A small wrapper function or macro like saturating_add_inplace(&mut b, a); would be better I think.

@t-nelson
Copy link
Contributor Author

t-nelson commented Jan 5, 2022

Two things:

* Have you run and measured these against MB? Because some of the metrics might measure so little that the measurement itself is more expensive.

You're right! I noticed some things were being measured that were unlikely to be expensive during a rebase. I'll re-evaluate the inherited commits

* There is a lot of `b = b.saturating_add(a);` which is not only hard to read but also error prone as it can lead to copy pasta like `d = b.saturating_add(a);`. A small wrapper function or macro like `saturating_add_inplace(&mut b, a);` would be better I think.

Will add. Jack wants it too, which is consensus enough for me 🙂

@t-nelson t-nelson merged commit 390ef0f into solana-labs:master Jan 6, 2022
@t-nelson t-nelson deleted the FixMetrics branch January 6, 2022 10:56
mergify bot added a commit that referenced this pull request Jan 7, 2022
* move `ExecuteTimings` from `runtime::bank` to `program_runtime::timings`

(cherry picked from commit 7d32909)

# Conflicts:
#	core/Cargo.toml
#	ledger/Cargo.toml
#	programs/bpf/Cargo.lock

* Add execute metrics

(cherry picked from commit b25e4a2)

* Add metrics for executor creation

(cherry picked from commit 848b6df)

* Add helper macro for `AddAssign`ing with saturating arithmetic

(cherry picked from commit deb9344)

* Use saturating_add_assign macro

(cherry picked from commit 72fc609)

* Consolidate process instruction execution timings to own struct

(cherry picked from commit 390ef0f)

Co-authored-by: Trent Nelson <trent@solana.com>
Co-authored-by: Carl Lin <carl@solana.com>
mergify bot added a commit that referenced this pull request Jan 18, 2022
* consolidate execute timings to a common module

* Add execute metrics

* Add metrics for executor creation

* Use saturating_add_assign macro

Co-authored-by: Trent Nelson <trent@solana.com>
Co-authored-by: Carl Lin <carl@solana.com>
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.

None yet

4 participants