-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[move-vm][aptos-vm] Basic execution counters #15086
Conversation
⏱️ 2h 54m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @georgemitenkov and the rest of your teammates on Graphite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with comments to be addressed.
9cc3114
to
68c5e3b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
68c5e3b
to
ffd1b69
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
||
/// Helper trait to encapsulate [HistogramVec] functionality. Users can use this trait to time | ||
/// different VM parts collecting metrics for different labels. Use wisely as timers do introduce | ||
/// an overhead, so using on extremely hot path is not recommended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is too optimistic of a wording, let's at least remove word "extremely". Even a moderate hot paths can cause slowdown, as histogram is not that cheap.
Things in this PR are probably fine (I assume type_to_type_tag is not called too often?), but it is easy do add overhead (i.e
Cache hot for data would probably be questionable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Was testing with e2e, data accesses are indeed something we do not want to record, as there is a bit of regression. At least recording gas traversal & loading/verification miss + total execution time should be enough
ffd1b69
to
fa283ca
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
Some basic counters that address #14348. Note that some metrics should not be collected via this timer, e.g., interpreter loop so that there is no perf degradation. Hence, added only in a few interesting places (e.g., module loading cache misses, resource loading cache misses, total execution times for user transaction with basic break downs).
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist