feat: Add metrics to measure compiled-contract cache hits/misses for chunk production vs. chunk validation. #11178

tayfunelmas · 2024-04-29T17:42:27Z

As part of #11125, we add metrics to count compiled-contract cache hits/misses.

We want to track the use of compiled-contract cache for apply-chunk operations. With stateless validation, apply-chunk will be executed for two different purposes: (1) while updating the shards by chunk producers as part of tracking shards and (2) while validating the chunk-witness in stateless validation (they do not necessarily track the shards they validate). Since the chunk validators are stateless, they will need to recompile contracts that they see anew unless they have already cached the previous compilation results.

We want to closely monitor the trend for chunk validators, thus we need to distinguish the apply-chunk operations executed for chunk producers vs chunk validators.

In this PR, we introduce a new enum to make this distinction and propagate it through apply-chunk call stack up to the point VM is called. Then we use the Metrics structure exported by the VM.run method to report the metrics.

tayfunelmas · 2024-04-29T17:47:42Z

Adding @nagisa to see if this approach makes sense.

runtime/near-vm-runner/src/near_vm_runner/metrics.rs

runtime/near-vm-runner/src/near_vm_runner/runner.rs

runtime/near-vm-runner/src/near_vm_runner/metrics.rs

…to update metrics.

…nly once.

tayfunelmas · 2024-04-30T21:38:24Z

runtime/runtime/src/actions.rs

            r
        }
-        res => res,
+        res => {
+            metrics.report(


moved this metrics.report call (for when we can find the compiled code in cache even without the contract code) here to avoid double counting cache lookups. metrics reset when running VM.run but are reported before the reset. please check.

We still want to report metrics even if there is a failure in execution, so something smarter needs to be done... but I'm also not-quite following the concern, zero out the values as necessary when reporting and/or returning the Metrics?

Got it. Updated the code to avoid double counting and only invoking the report_metrics once. I call reset_metrics at the beginning and report_metrics at the end (to cover both calls to VM.run). PTAL.

runtime/near-vm-runner/src/metrics.rs

nagisa · 2024-05-01T15:38:04Z

runtime/near-vm-runner/src/metrics.rs

 #[derive(Default, Copy, Clone)]
 pub struct Metrics {
    near_vm_compilation_time: Duration,
    wasmtime_compilation_time: Duration,
+    /// True iff the runtime checked the compiled contract cache and found already-compiled code.
+    compiled_contract_cache_hit: Option<bool>,


I'd use a usize that counts the count. It is less error prone this way (e.g. what happens if the code is adjusted to access the cache multiple times?)

It might also solve your concerns with double counting if we make fn report operate on the value inside the TLS... So we would change the return value of run back to only return an VMResult, and then change fn report to be a static method or something that we would operate on the TLS value much like the increment functions do.

Yeah I like that. I changed report to a static function (renamed to report_metrics) and also renamed reset to reset_metrics. And calling reset_metrics and report_metrics once in the VM runner (outside of VM.run). PTAL.

into compile-metric

nagisa

LGTM!

…or near_vm only.

…nabled for MacOS and we get function not used error).

codecov · 2024-05-06T21:53:08Z

Codecov Report

Attention: Patch coverage is 92.70833% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 71.21%. Comparing base (a0c1466) to head (b129a73).

Files	Patch %	Lines
runtime/near-vm-runner/src/metrics.rs	90.56%	3 Missing and 2 partials ⚠️
core/primitives-core/src/apply.rs	87.50%	1 Missing ⚠️
tools/state-viewer/src/commands.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11178      +/-   ##
==========================================
+ Coverage   71.19%   71.21%   +0.01%     
==========================================
  Files         783      784       +1     
  Lines      155172   155243      +71     
  Branches   155172   155243      +71     
==========================================
+ Hits       110471   110549      +78     
+ Misses      39960    39955       -5     
+ Partials     4741     4739       -2

Flag	Coverage Δ
backward-compatibility	`0.24% <0.00%> (-0.01%)`	⬇️
db-migration	`0.24% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.41% <0.00%> (-0.01%)`	⬇️
integration-tests	`37.00% <83.33%> (+0.09%)`	⬆️
linux	`69.19% <90.62%> (+<0.01%)`	⬆️
linux-nightly	`70.68% <92.70%> (+0.03%)`	⬆️
macos	`53.12% <20.48%> (-0.02%)`	⬇️
pytests	`1.63% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.42% <0.00%> (-0.01%)`	⬇️
unittests	`65.95% <77.89%> (+0.01%)`	⬆️
upgradability	`0.29% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tayfunelmas added 5 commits April 26, 2024 16:36

Initial commit.

1f8cff8

Connect pieces second commit.

11e66e1

Move reason to primitives and not use string.

deccb30

Merge branch 'master' into compile-metric

162e583

Change string to ApplyChunkReason in ApplyState

005cde2

tayfunelmas requested a review from nagisa April 29, 2024 17:47

tayfunelmas added 2 commits April 29, 2024 11:48

Address clippy findings.

eec1b28

Merge branch 'master' into compile-metric

7436146

tayfunelmas marked this pull request as ready for review April 29, 2024 18:52

tayfunelmas requested a review from a team as a code owner April 29, 2024 18:52

tayfunelmas closed this Apr 29, 2024

tayfunelmas deleted the compile-metric branch April 29, 2024 19:22

tayfunelmas restored the compile-metric branch April 29, 2024 19:23

tayfunelmas reopened this Apr 29, 2024

nagisa requested changes Apr 30, 2024

View reviewed changes

runtime/near-vm-runner/src/near_vm_runner/metrics.rs Outdated Show resolved Hide resolved

runtime/near-vm-runner/src/near_vm_runner/runner.rs Outdated Show resolved Hide resolved

runtime/near-vm-runner/src/near_vm_runner/metrics.rs Outdated Show resolved Hide resolved

tayfunelmas added 5 commits April 30, 2024 10:57

Merge branch 'master' into compile-metric

c3e8ccd

Remove apply_reason from VContext and use exported Metrics structure …

4347216

…to update metrics.

fix clippy errors and format

9857410

Add one more cache hit record.

15d66a9

Merge hit and miss functions and also move metrics.record to record o…

b4dfbf1

…nly once.

tayfunelmas commented Apr 30, 2024

View reviewed changes

nagisa reviewed May 1, 2024

View reviewed changes

runtime/near-vm-runner/src/metrics.rs Outdated Show resolved Hide resolved

nagisa reviewed May 1, 2024

View reviewed changes

tayfunelmas added 6 commits May 1, 2024 11:25

Address comments v1

7beb96d

Make reset and report operations static

d975a96

Set the metric correctly to consider memory and disk cache

292b613

Merge branch 'master' into compile-metric

35f9ec9

Remove extra call to record_compiled_contract_cache_lookup

565dd0c

Merge branch 'compile-metric' of https://github.com/tayfunelmas/nearcore

a68c34e

into compile-metric

nagisa approved these changes May 1, 2024

View reviewed changes

tayfunelmas enabled auto-merge May 2, 2024 01:17

tayfunelmas added 6 commits May 3, 2024 16:38

Merge branch 'master' into compile-metric

e720ebd

Merge branch 'master' into compile-metric

60617ad

Merge branch 'master' into compile-metric

17f0a9b

Merge branch 'master' into compile-metric

21886bf

Add instrumentation to enable record_compiled_contract_cache_lookup f…

7d60706

…or near_vm only.

Also add x86_64 annotation to skip MacOS tests (the codepath is not e…

b129a73

…nabled for MacOS and we get function not used error).

tayfunelmas added this pull request to the merge queue May 6, 2024

Merged via the queue into near:master with commit 72a159b May 6, 2024
29 checks passed

tayfunelmas deleted the compile-metric branch May 6, 2024 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add metrics to measure compiled-contract cache hits/misses for chunk production vs. chunk validation. #11178

feat: Add metrics to measure compiled-contract cache hits/misses for chunk production vs. chunk validation. #11178

tayfunelmas commented Apr 29, 2024 •

edited

Loading

tayfunelmas commented Apr 29, 2024

tayfunelmas Apr 30, 2024 •

edited

Loading

nagisa May 1, 2024 •

edited

Loading

tayfunelmas May 1, 2024 •

edited

Loading

nagisa May 1, 2024 •

edited

Loading

tayfunelmas May 1, 2024

nagisa left a comment

codecov bot commented May 6, 2024

feat: Add metrics to measure compiled-contract cache hits/misses for chunk production vs. chunk validation. #11178

feat: Add metrics to measure compiled-contract cache hits/misses for chunk production vs. chunk validation. #11178

Conversation

tayfunelmas commented Apr 29, 2024 • edited Loading

tayfunelmas commented Apr 29, 2024

tayfunelmas Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

nagisa May 1, 2024 • edited Loading

Choose a reason for hiding this comment

tayfunelmas May 1, 2024 • edited Loading

Choose a reason for hiding this comment

nagisa May 1, 2024 • edited Loading

Choose a reason for hiding this comment

tayfunelmas May 1, 2024

Choose a reason for hiding this comment

nagisa left a comment

Choose a reason for hiding this comment

codecov bot commented May 6, 2024

Codecov Report

tayfunelmas commented Apr 29, 2024 •

edited

Loading

tayfunelmas Apr 30, 2024 •

edited

Loading

nagisa May 1, 2024 •

edited

Loading

tayfunelmas May 1, 2024 •

edited

Loading

nagisa May 1, 2024 •

edited

Loading