[execution] Improve caching in accessors + simplify code by dmtrskv · Pull Request #958 · NilFoundation/nil

dmtrskv · 2025-06-11T21:32:15Z

State accessor appears in profiles as an active creator of objects in memory.
Here I simplify the code of the accessor tailoring it to the current needs. The caching is reworked and improved (it wasn't happening at some places at all).

There are also some simplifications in the other code, especially improvements in the usage of accessors.

Un1oR · 2025-06-12T09:18:51Z

nil/internal/db/accessors.go

+func GetFromShard(tx RoTx, shardId types.ShardId, table ShardedTableName, key prettyKey) ([]byte, error) {
+	data, err := tx.GetFromShard(shardId, table, key.Bytes())
+	if err != nil {
+		return nil, fmt.Errorf("%w: key=%s", err, key)


Perhaps shardId we also want to add to the error message?

Moved to #969

Un1oR · 2025-06-12T09:23:23Z

nil/internal/db/badger.go

 	item, err := tx.tx.Get(MakeKey(tableName, key))
 	if errors.Is(err, badger.ErrKeyNotFound) {
-		return nil, ErrKeyNotFound
+		return nil, fmt.Errorf("%w: table %s", ErrKeyNotFound, tableName)


We have quite a few checks for errors.Is(err, db.ErrKeyNotFound). I think we might break some logic with such changes. If we want to add context to ErrrKeyNotFound, perhaps we should make it a full type and change the errors.Is checks to errors.As. Also, it might be better to make such changes as a separate PR.

Removed it here and added this to the Gets in accessors.
Wrapping errors this way doesn't affect errors.Is.

Moved these changes to another PR: #969

Un1oR · 2025-06-12T09:30:39Z

nil/internal/execution/state_accessor.go

-	outTxCountsLRU     *lru.Cache[common.Hash, [][]byte]
-	receiptsLRU        *lru.Cache[common.Hash, [][]byte]
-}
+func newRawAccessorCache(blocksLRUSize, txnRLUSize int) *cache {


Suggested change

func newRawAccessorCache(blocksLRUSize, txnRLUSize int) *cache {

func newRawAccessorCache(blocksLRUSize, txnLRUSize int) *cache {

Un1oR · 2025-06-12T09:45:34Z

nil/internal/execution/state_accessor.go

-	b.withDbTimestamp = true
-	return b
-}
+	res, err := db.ReadBlockBytes(s.tx, s.shardId, hash)


We could also check s.cache.blocksLRU

Reworked it a bit.

Un1oR · 2025-06-12T09:48:32Z

nil/internal/execution/state_accessor.go

 			return nil, err
 		}
-		sa.cache.blocksLRU.Add(hash, block)
+		return &types.RawBlockWithExtractedData{


I don't really like this use of a type without filling in some of the fields. If Go were a normal language, we should use variant here, but since we have what we have, let's keep some form of rawBlockAccessorResult to represent the mandatory + optional part of the data fields.

I based it on the usages, but they actually could be improved/simplified. So, did that.

Un1oR · 2025-06-12T09:57:09Z

nil/internal/execution/state_accessor.go

-	check.PanicIfErr(err)
+type cache struct {
+	blocksLRU       *lru.Cache[common.Hash, *types.Block]
+	fullBlocksLRU   *lru.Cache[common.Hash, *types.RawBlockWithExtractedData]


Have you considered a more normalized way of storing data, where the ExtractedData part is stored separately, and rawBlocksLRU is used for both raw headers and full blocks?

Then I'll have to introduce ExtractedData, etc. Having an extra link to a block header is not a problem.
Instead, I merged raw and decoded headers, since they are unlikely to be used one without the other (it can only happen with the raw ones and only in a single rpc method).

Un1oR · 2025-06-12T10:00:54Z

nil/internal/execution/state_accessor.go

-	withDbTimestamp     bool
-	withConfig          bool
-}
+func (s RawShardAccessor) decodeBlock(hash common.Hash, data []byte) (*types.Block, error) {


For some reason I really want to see in the function name the information that it is caching.

Suggested change

func (s RawShardAccessor) decodeBlock(hash common.Hash, data []byte) (*types.Block, error) {

func (s RawShardAccessor) decodeBlockCached(hash common.Hash, data []byte) (*types.Block, error) {

Probably because otherwise it's very easy to think from the name that it's pure, and make incorrect assumptions about behavior by reading the call locations.

Agree, but removed this method altogether.

Un1oR · 2025-06-12T11:09:38Z

nil/services/rpc/rawapi/internal/local_block.go

-			if len(errMsg) > 0 {
-				result.Errors[txnHash] = errMsg
+		if len(errMsg) > 0 {
+			if result.Errors == nil {


How about building a temporary dictionary, and assigning it to result.Errors at the end if it's not empty? Or conversely, assigning nil to result.Errors if it's empty. By the way, why do we even want to maintain such an invariant?

I thought it would be nice not to touch the result, unless there are actual errors.

Un1oR · 2025-06-12T11:15:42Z

nil/internal/collate/proposer_test.go

-		s.checkReceipt(shardId, m)
-	})
-
-	s.Run("CheckRefundsSeqno", func() {


Why did we delete this test?

It doesn't have anything to do with the proposer functionality.

Gezort · 2025-06-26T08:37:38Z

nil/internal/execution/state_accessor.go

+	blockHashByNumber *BlockHashByNumberAccessor
 }

 func NewStateAccessor(blockLRUSize, txnLRUSize int) *StateAccessor {


btw if you wanna improve cache it may make sense to implement slightly different algo than simple LRU

https://en.wikipedia.org/wiki/Adaptive_replacement_cache - this one seems to be quite efficient
https://github.com/alexanderGugel/arc - and here's some implementation in go

dmtrskv temporarily deployed to prod June 11, 2025 21:32 — with GitHub Actions Inactive

dmtrskv temporarily deployed to prod June 11, 2025 22:20 — with GitHub Actions Inactive

dmtrskv force-pushed the state-accessor-rework branch from 22aee9a to 216db6c Compare June 11, 2025 23:12

dmtrskv temporarily deployed to prod June 11, 2025 23:13 — with GitHub Actions Inactive

Un1oR reviewed Jun 12, 2025

View reviewed changes

dmtrskv force-pushed the state-accessor-rework branch from 216db6c to f41374e Compare June 18, 2025 15:28

dmtrskv had a problem deploying to prod June 18, 2025 15:29 — with GitHub Actions Failure

dmtrskv force-pushed the state-accessor-rework branch from f41374e to 2bfd100 Compare June 18, 2025 16:13

dmtrskv had a problem deploying to prod June 18, 2025 16:14 — with GitHub Actions Failure

dmtrskv force-pushed the state-accessor-rework branch from 2bfd100 to ba582e6 Compare June 18, 2025 16:34

dmtrskv temporarily deployed to prod June 18, 2025 16:34 — with GitHub Actions Inactive

dmtrskv marked this pull request as ready for review June 18, 2025 17:24

dmtrskv requested a review from Un1oR June 18, 2025 17:28

dmtrskv marked this pull request as draft June 18, 2025 19:20

dmtrskv force-pushed the state-accessor-rework branch from ba582e6 to bc76ef8 Compare June 23, 2025 13:44

dmtrskv temporarily deployed to prod June 23, 2025 13:44 — with GitHub Actions Inactive

dmtrskv force-pushed the state-accessor-rework branch from bc76ef8 to 0ffafe2 Compare June 23, 2025 17:04

dmtrskv temporarily deployed to prod June 23, 2025 17:04 — with GitHub Actions Inactive

dmtrskv force-pushed the state-accessor-rework branch from 0ffafe2 to 200bbca Compare June 24, 2025 14:48

dmtrskv temporarily deployed to prod June 24, 2025 14:48 — with GitHub Actions Inactive

dmtrskv force-pushed the state-accessor-rework branch from 200bbca to 528bc74 Compare June 24, 2025 16:16

dmtrskv temporarily deployed to prod June 24, 2025 16:16 — with GitHub Actions Inactive

dmtrskv force-pushed the state-accessor-rework branch from 528bc74 to 0f8e20c Compare June 24, 2025 16:32

dmtrskv had a problem deploying to prod June 24, 2025 16:42 — with GitHub Actions Failure

dmtrskv force-pushed the state-accessor-rework branch from 0f8e20c to d4b066d Compare June 25, 2025 12:15

dmtrskv had a problem deploying to prod June 25, 2025 12:15 — with GitHub Actions Error

dmtrskv force-pushed the state-accessor-rework branch from d4b066d to bf0abe8 Compare June 25, 2025 12:28

dmtrskv temporarily deployed to prod June 25, 2025 12:28 — with GitHub Actions Inactive

dmtrskv mentioned this pull request Jun 25, 2025

[cache] BlockAccessor with caching separate from StateAccessor #980

Open

[cache] BlockAccessor with caching separate from StateAccessor

b26ad96

[execution] Improve caching in accessors + simplify code

dffdbbb

dmtrskv force-pushed the state-accessor-rework branch from bf0abe8 to dffdbbb Compare June 25, 2025 17:22

dmtrskv temporarily deployed to prod June 25, 2025 17:22 — with GitHub Actions Inactive

dmtrskv changed the base branch from main to block-accessor June 25, 2025 17:22

dmtrskv marked this pull request as ready for review June 25, 2025 17:26

olegrok approved these changes Jun 26, 2025

View reviewed changes

Gezort reviewed Jun 26, 2025

View reviewed changes

dmtrskv force-pushed the block-accessor branch from b26ad96 to 8ee52e1 Compare June 27, 2025 14:37

	func newRawAccessorCache(blocksLRUSize, txnRLUSize int) *cache {
	func newRawAccessorCache(blocksLRUSize, txnLRUSize int) *cache {

	func (s RawShardAccessor) decodeBlock(hash common.Hash, data []byte) (*types.Block, error) {
	func (s RawShardAccessor) decodeBlockCached(hash common.Hash, data []byte) (*types.Block, error) {

Conversation

dmtrskv commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants