Skip to content

StorageValue #8482

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

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

StorageValue #8482

wants to merge 40 commits into from

Conversation

Scooletz
Copy link
Contributor

@Scooletz Scooletz commented Apr 8, 2025

This PR introduces a notion of StorageValue that represents a non-trimmed 32-byte long value persisted contracts' storage. The main goal is to remove the clutter of new byte[] that was used to keep the values and track the changes for the storage, and replace it with a simple struct. To make it work a storage is required to ammortize all data structures that previously hold a ref to byte[] and now need to keep the StorageValue. The storage for it is called StorageValueMap and allows mapping an arbitrary value of StorageValue to a wrapped Ptr struct that is a size of a ref/ptr.

The pointer is used in the storage cache, so that ConcurrentDictionary nodes should be smaller now (value is 8 bytes instead of 32) and should use less memory. They also directly return a platform size value so less copying will occur.

The actual size of the amendment in VirtualMachine is small, as only SLOAD/SSTORE TLOAD/TSTORE are affected. Additionally the tracing seam has been strongly amended.

One more area that is touched is a different implementation of .WithoutLeadingZeros() that is ~ 1.5 times faster that the
Bytes.WithoutLeadingZeros(). It uses the fact that we know the length and for 32 bytes it's easy to vectorize it.

Additional cost of this approach is 36MB of memory that is never released, cleared or GCed.

Benchmarks

A set of benchmarks, using storage related methods: reset, reading with block cache warmed up and a cold cache. Additional set is added showing that the mapping penalty needs to be paid somewhere.

The benchmarks use .IsZero for making some operation on the retrieved value. The new IsZero is quite fast as all it does is

; Nethermind.Core.StorageValue.get_IsZero()
       vmovups   ymm0,[rcx]
       vptest    ymm0,ymm0
       sete      al
       movzx     eax,al
       vzeroupper
       ret
; Total bytes of code 19

Micro-benchmarks

Before
Method Mean Error StdDev Gen0 Code Size Allocated
Just_reset 11.38 ns 0.160 ns 0.149 ns - 1,410 B -
PreCached_small_indexes 119.46 ns 1.379 ns 1.152 ns 0.0095 6,310 B 122 B
NotCached_small_indexes 822.13 ns 10.680 ns 9.990 ns 0.1078 4,642 B 1354 B
Set 70.04 ns 0.400 ns 0.354 ns 0.0057 1,840 B 72 B
After
Method Mean Error StdDev Gen0 Code Size Allocated
Just_reset 11.16 ns 0.079 ns 0.070 ns - 1,434 B -
PreCached_small_indexes 81.34 ns 0.811 ns 0.758 ns 0.0082 6,083 B 104 B
NotCached_small_indexes 641.38 ns 3.634 ns 3.034 ns 0.0830 4,468 B 1050 B
Set 75.41 ns 0.722 ns 0.676 ns 0.0057 2,342 B 72 B

CoarseGrained

Before
Method Bytecode Mean Error StdDev Gen0 Gen1 Allocated
Execute Byte[14] 4.257 us 0.0385 us 0.0360 us 0.2823 - 3.46 KB
Execute Byte[27] 7.250 us 0.0678 us 0.0634 us 0.5569 0.0076 6.87 KB
Execute Byte[36] 2.883 us 0.0120 us 0.0112 us 0.1488 - 1.84 KB
After
Method Bytecode Mean Error StdDev Gen0 Gen1 Allocated
Execute Byte[14] 3.960 us 0.0248 us 0.0220 us 0.2823 - 3.46 KB
Execute Byte[27] 6.938 us 0.0735 us 0.0652 us 0.5493 0.0076 6.74 KB
Execute Byte[36] 2.671 us 0.0237 us 0.0221 us 0.1373 - 1.71 KB

Running nodes

Comparison using GitHub action for RPC (comparable hardware)

image

Changes

  • removal of Span<byte> and byte[] based code for storage
  • introduction of a readonly struct StorageValue and its pointy counterpart Ptr
  • introduction of ref returning and ref accepting API for storage related operations
  • amendments in VirtualMachine in storage management opcodes, including gas calculations

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Compare on various sizes of VMs

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

@Scooletz
Copy link
Contributor Author

Scooletz commented Apr 9, 2025

A bit related to #8442 although working on a different level.

@Scooletz
Copy link
Contributor Author

Let's pause reviews for a while. Using more comparable machines showed different results from the ones presented before, showing that this branch is slower.

@Scooletz Scooletz force-pushed the storage-value branch 3 times, most recently from b4d9ce1 to e021927 Compare April 11, 2025 11:04
@Scooletz Scooletz mentioned this pull request Apr 24, 2025
8 tasks
@Scooletz Scooletz marked this pull request as ready for review April 24, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants