[Config Change] Stylus LRU cache with bytes capacity instead of number of entries capacity #2595

diegoximenes · 2024-08-20T17:58:05Z

Stylus LRU cache with bytes capacity instead of number of entries capacity.
It infers the size of each entry by computing the number of bytes related to the serialized wasm module.

This PR also removes -Wall from cgo CFLAGS.
go doesn't handle it well: https://stackoverflow.com/questions/56633817/warning-unused-variable-cgo-a.
With it the following warning was being generated without need:

cgo-gcc-prolog: In function ‘_cgo_80afc8d9354c_Cfunc_stylus_clear_lru_cache’:
cgo-gcc-prolog:139:49: warning: unused variable ‘_cgo_a’ [-Wunused-variable]

diegoximenes · 2024-08-22T13:13:25Z

execution/gethexec/blockchain.go

@@ -75,7 +75,7 @@ var DefaultCachingConfig = CachingConfig{
 	SnapshotRestoreGasLimit:            300_000_000_000,
 	MaxNumberOfBlocksToSkipStateSaving: 0,
 	MaxAmountOfGasToSkipStateSaving:    0,
-	StylusLRUCache:                     256,
+	StylusLRUCacheCapacity:             256,


Suggestions on the default value?

I am fine with 256 for now. If we frequently run out, we can change it in a later PR.

tsahee

realised I'm sitting on these Pending comments for a while .. sorry

arbitrator/stylus/src/cache.rs

tsahee · 2024-08-23T20:42:53Z

arbitrator/stylus/src/cache.rs

+        let count = cache.lru.len();
+        let metrics = LruCacheMetrics {
+            // add 1 to each entry to account that we subtracted 1 in the weight calculation
+            size_bytes: (cache.lru.weight() + count).try_into().unwrap(),


Normal entry is order of magnitude of 100,000. a few bytes per entry isn't worth fussing over (I'd go with weight = deser_size + 128 and return lru.weight() from here)

Added the 128 bytes to account for overheads that you mentioned :).

I kept taking into account that clru defines that each entry consumes (weight + 1) of the cache capacity though.
This simplifies tests that checks the expected cache size.

making tests simpler is a worthy goal.
Won't you get the same result if you don't decrease 1 from each weight and don't add count here?

(if not - why? if so - separate PR)

Nope :(.

clru defines that each entry will consume weight(entry) + 1 of the cache's capacity.
We define that weight(entry) = size_in_bytes - 1, in that way each entry will actually consume size_in_bytes of the cache's capacity.
This +1/-1 doesn't make much of a practical difference in production, but with it the cache's interface is more straightforward to understand, since we define the cache capacity's in number of bytes, and each entry consumes the number of bytes related to it.
A more straightforward interface also simplifies testing.

That said, considering that we are decreasing 1 from each weight, we add 1 one here so the metrics are consistent.
But it wouldn't be so harmful if this metric is off by one for each entry.
But since we can be precise here without compromising much of code simplicity, I don't see why we wouldn't do this :).

arbitrator/stylus/src/lib.rs

arbos/programs/native.go

execution/gethexec/blockchain.go

eljobe · 2024-08-29T08:06:21Z

execution/gethexec/blockchain.go

@@ -75,7 +75,7 @@ var DefaultCachingConfig = CachingConfig{
 	SnapshotRestoreGasLimit:            300_000_000_000,
 	MaxNumberOfBlocksToSkipStateSaving: 0,
 	MaxAmountOfGasToSkipStateSaving:    0,
-	StylusLRUCache:                     256,
+	StylusLRUCacheCapacity:             256,


I am fine with 256 for now. If we frequently run out, we can change it in a later PR.

execution/gethexec/executionengine.go

eljobe

LGTM

gligneul

LGTM

arbos/programs/native.go

arbitrator/stylus/src/cache.rs

…us LRU cache entry

…trics

tsahee · 2024-09-26T15:44:46Z

arbitrator/stylus/src/cache.rs

+        let count = cache.lru.len();
+        let metrics = LruCacheMetrics {
+            // add 1 to each entry to account that we subtracted 1 in the weight calculation
+            size_bytes: (cache.lru.weight() + count).try_into().unwrap(),


making tests simpler is a worthy goal.
Won't you get the same result if you don't decrease 1 from each weight and don't add count here?

tsahee · 2024-09-26T15:46:35Z

arbitrator/stylus/src/cache.rs

+pub struct LruCounters {
+    pub hits: u32,
+    pub misses: u32,
+    pub does_not_fit: u32,


can we add a counter for long-term cache size and add/remove form it when we add/remove entries to the hashmap?

(separate PR)

tsahee · 2024-09-26T23:51:46Z

execution/gethexec/executionengine.go

@@ -963,4 +963,15 @@ func (s *ExecutionEngine) Start(ctx_in context.Context) {
 			}
 		}
 	})
+	// periodically update stylus lru cache metrics
+	s.LaunchThread(func(ctx context.Context) {


Lets add a boolean config weather or not to collect metrics. On by default, off by default for system-tests.

(separate PR)

But what is the rationale of not always enabling those metrics?
Is it some worry about Prometheus performance with the number of metrics increase?

Sometimes I think that too granular configs, like this one, are not so useful, increases code complexity, and unnecessarily creates confusion for users, that will see one more config that could be "optimized" .

The rational is that the stylus cache is static and in tests I don't want multiple parallel tests measuring the same PR.
I don't plan to ever turn it off in binary, just tests.

tsahee · 2024-09-27T00:22:42Z

arbitrator/stylus/src/cache.rs

+        let count = cache.lru.len();
+        let metrics = LruCacheMetrics {
+            // add 1 to each entry to account that we subtracted 1 in the weight calculation
+            size_bytes: (cache.lru.weight() + count).try_into().unwrap(),


(if not - why? if so - separate PR)

cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Aug 20, 2024

diegoximenes mentioned this pull request Aug 20, 2024

AsmSizeEstimateKb in CacheWasmRust OffchainLabs/go-ethereum#351

Closed

diegoximenes force-pushed the stylus_cache_bytes_capacity branch 3 times, most recently from 2961c11 to b90c32a Compare August 22, 2024 13:08

diegoximenes commented Aug 22, 2024

View reviewed changes

diegoximenes changed the title ~~Stylus LRU cache with kilobytes capacity instead of number of entries capacity~~ [Config Change] Stylus LRU cache with kilobytes capacity instead of number of entries capacity Aug 22, 2024

diegoximenes marked this pull request as ready for review August 22, 2024 14:58

diegoximenes changed the title ~~[Config Change] Stylus LRU cache with kilobytes capacity instead of number of entries capacity~~ [Config Change] Stylus LRU cache with bytes capacity instead of number of entries capacity Aug 22, 2024

diegoximenes requested review from eljobe, gligneul and magicxyyz August 22, 2024 14:58

diegoximenes force-pushed the stylus_cache_bytes_capacity branch from e91655c to 165d484 Compare August 27, 2024 13:31

tsahee reviewed Aug 27, 2024

View reviewed changes

tsahee reviewed Aug 28, 2024

View reviewed changes

arbitrator/stylus/src/lib.rs Show resolved Hide resolved

tsahee reviewed Aug 28, 2024

View reviewed changes

arbos/programs/native.go Show resolved Hide resolved

diegoximenes requested a review from tsahee August 28, 2024 20:49

eljobe previously approved these changes Aug 29, 2024

View reviewed changes

diegoximenes dismissed eljobe’s stale review via b514e73 August 29, 2024 13:30

diegoximenes requested a review from eljobe August 29, 2024 13:32

eljobe previously approved these changes Aug 30, 2024

View reviewed changes

gligneul previously approved these changes Sep 5, 2024

View reviewed changes

arbos/programs/native.go Show resolved Hide resolved

arbitrator/stylus/src/cache.rs Show resolved Hide resolved

diegoximenes added 8 commits September 6, 2024 10:17

First step in lru to lru_mem migration in stylus cache

fab821e

stylus-lru-cache-size instead of stylus-lru-cache flag

e4c9406

Uses asm size estimate to define amount of heap memory used by a styl…

a3b2dd7

…us LRU cache entry

Basic stylus cache metrics

7e0de89

clru instead of lru-mem

9bc34de

Simplifies stylus lru cache metrics retrieval

f4c723c

AsmSizeEstimate to AsmSizeEstimateKb, CLI rust cache size flag in Mb

098fca5

Fixes weight of stylus cache entry

6261168

diegoximenes added 21 commits September 6, 2024 10:49

Adds comment on periodic procedure that retrieves stylus lru cache me…

fa9a932

…trics

Updates comment TestWasmLruCache

d9bf584

Renames StylusLRUCacheSizeMb to StylusLRUCacheSize

ce46676

Updates lru cache metrics data sizes

7cb4fa1

Simplifies weight computation by using saturating_sub

21217f1

Uses if let instead of match

7e6b759

Removes unused return

fe2377c

Creates a DOES_NOT_FIT_MSG static

e439bc8

Uses is_err instead of let Err(_)

281f608

Format rs files

a2f1b60

Relies on module.serialize to estimate asm size

761f84a

Renames resize to set capacity in stylus cache

21664e9

Don't hardcode wasm sizes in TestWasmLruCache

b7a949c

Removes unnecessary calls to ExecutionEngine regarding Wasm LRU cache

bbed30a

Removes unsafe from stylus_get_asm_size_estimate_bytes

17a62da

Format rs files

8a3d6ef

From println to eprintln in stylus cache

eaa04db

Renames asm_size_estimate_bytes to lru_entry_size_estimate_bytes

1449a7b

Adds extra bytes to lru_entry_size_bytes to account for overheads

ba9e9d1

Format rs files

5a82f00

Renames rustCacheCapacityMb to rustCacheCapacityMB

f144eb1

diegoximenes dismissed stale reviews from gligneul and eljobe via f144eb1 September 6, 2024 18:39

diegoximenes force-pushed the stylus_cache_bytes_capacity branch from b514e73 to f144eb1 Compare September 6, 2024 18:39

tsahee approved these changes Sep 27, 2024

View reviewed changes

tsahee added the design-approved label Sep 27, 2024

Merge branch 'master' into stylus_cache_bytes_capacity

b4cb8ec

tsahee enabled auto-merge September 27, 2024 00:23

tsahee merged commit 8230432 into master Sep 27, 2024
16 checks passed

tsahee deleted the stylus_cache_bytes_capacity branch September 27, 2024 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Config Change] Stylus LRU cache with bytes capacity instead of number of entries capacity #2595

[Config Change] Stylus LRU cache with bytes capacity instead of number of entries capacity #2595

diegoximenes commented Aug 20, 2024 •

edited

Loading

diegoximenes Aug 22, 2024

eljobe Aug 29, 2024

tsahee left a comment

tsahee Aug 23, 2024

diegoximenes Aug 28, 2024

tsahee Sep 26, 2024

tsahee Sep 27, 2024

diegoximenes Sep 27, 2024

eljobe Aug 29, 2024

eljobe left a comment

gligneul left a comment

tsahee Sep 26, 2024

tsahee Sep 26, 2024

tsahee Sep 27, 2024

diegoximenes Sep 27, 2024

tsahee Sep 26, 2024

tsahee Sep 27, 2024

diegoximenes Sep 27, 2024

tsahee Sep 27, 2024

tsahee Sep 27, 2024

[Config Change] Stylus LRU cache with bytes capacity instead of number of entries capacity #2595

[Config Change] Stylus LRU cache with bytes capacity instead of number of entries capacity #2595

Conversation

diegoximenes commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsahee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljobe left a comment

Choose a reason for hiding this comment

gligneul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoximenes commented Aug 20, 2024 •

edited

Loading