-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Add cache to stf context #19875
Add cache to stf context #19875
Conversation
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
server/v2/stf/stf.go
Outdated
@@ -391,6 +391,7 @@ type executionContext struct { | |||
meter gas.Meter | |||
events []event.Event | |||
sender []transaction.Identity | |||
Cache ModuleContainer |
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.
will this be a source of concurrency issues, when parrallized txs become a thing?
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.
do you mean by using the same ctx for many txs?
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.
u right we use clone ctx through makeContext
, need a way to update actual ctx after exec tx.
Let me adjust
@@ -349,6 +357,7 @@ | |||
if err != nil { | |||
return nil, nil, err | |||
} | |||
applyContextCache(ctx, ebCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
@@ -338,6 +345,7 @@ | |||
event.Attribute{Key: "mode", Value: "BeginBlock"}, | |||
) | |||
} | |||
applyContextCache(ctx, ebCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
@@ -314,6 +320,7 @@ | |||
event.Attribute{Key: "mode", Value: "BeginBlock"}, | |||
) | |||
} | |||
applyContextCache(ctx, bbCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
@@ -297,6 +302,7 @@ | |||
event.Attribute{Key: "mode", Value: "PreBlock"}, | |||
) | |||
} | |||
applyContextCache(ctx, pbCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
@@ -281,6 +285,7 @@ | |||
} | |||
msgResps[i] = resp | |||
} | |||
applyContextCache(ctx, execCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
@@ -258,6 +261,7 @@ | |||
if applyErr != nil { | |||
return nil, 0, nil, applyErr | |||
} | |||
applyContextCache(ctx, postTxCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
@@ -240,6 +242,7 @@ | |||
if applyErr != nil { | |||
return nil, 0, nil, applyErr | |||
} | |||
applyContextCache(ctx, postTxCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
@@ -205,6 +206,7 @@ | |||
if err != nil { | |||
return 0, nil, err | |||
} | |||
applyContextCache(ctx, validateCtx) |
Check warning
Code scanning / gosec
Errors unhandled.
I have initialized the empty cache for ctx with each One problem I'm facing is that the cache is only written by What if we modified the wdut? @tac0turtle |
Yea this is needed!! |
Adjusted in 29eb7d2 |
collections/item.go
Outdated
@@ -6,11 +6,15 @@ import ( | |||
"fmt" | |||
|
|||
"cosmossdk.io/collections/codec" | |||
"cosmossdk.io/server/v2/stf" |
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.
This seems problematic, collections should not depend on server v2 and STF.
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.
Ty, Wrapped in a container.Service
. Now collections is not depended on stf
.
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.
collections
should not depend on server/v2
server/v2/stf/go.mod
Outdated
|
||
require ( | ||
cosmossdk.io/core v0.11.0 | ||
cosmossdk.io/collections v0.4.0 |
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.
can this be removed?
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.
hmm it's using for the test
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 have very limited context on the new stf but I am bit worried about dirty reads in the caching approach. I could not find where state is reverted when a CacheMultiStore
is discarded. Also the cache seems to be reused via applyContextCache
by different methods. But I may got that wrong.
I found caching very hard to get right in the past. Especially on the app level. There should be caching on the store level already. It would be good to see some bechmarks how much another cache adds on the app level to this.
Ideally, caches come with configuration options so that people can trade memory for cpu and some metrics that show internals like usage size and hit rate.
// Package branch contains the core branch service interface. | ||
package container | ||
|
||
type Service interface { |
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.
personal preference: Service is a very overused name. Naming is hard but In this context, I assume it is just an abstract cache?
Would it make sense to move the interface to collections
package?
type Item[V any] Map[noKey, V] | ||
type Item[V any] struct { | ||
m Map[noKey, V] | ||
getContainer func(ctx context.Context) container.Service |
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.
q: why is it called container and not cache?
|
||
func NewModuleContainer() ModuleContainer { | ||
return ModuleContainer{ | ||
m: make(map[string]Container), |
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.
note: this cache is not limited. In address package for example simplelru.NewLRU
is used. Would it make sense here, too?
The cache is not safe for concurrent access. Looking into the future, would it make sense to support parallel execution?
func applyContextCache(dst, src context.Context) error { | ||
srcExecutionCtx, ok := src.(*executionContext) | ||
if !ok { | ||
return fmt.Errorf("Can not convert ctx to executionContext") |
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.
Looks like the errors are not handled by the callers. Would it make sense to drop them here already?
Thanks for reviewing. I agree this is hard. The idea here is to cache decoded values. You are right there are caches at the store level and within the database. We have found for certain items decoding is a large overhead and done many times during a block. This is meant to reduce this |
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.
We should make sure this works in the parallel execution case as well.
return ctx.(*executionContext) | ||
} | ||
|
||
func NewExecutionContext() *executionContext { |
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.
these things should not be exported
collections/schema.go
Outdated
return NewSchemaBuilderFromAccessor(service.OpenKVStore) | ||
sb := NewSchemaBuilderFromAccessor(service.OpenKVStore) | ||
kl, ok := service.(interface { | ||
OpenContainer(ctx context.Context) container.Service |
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.
this interface should be in core
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.
resolved
Co-authored-by: Marko <marko@baricevic.me>
return item | ||
} | ||
|
||
// Get gets the item, if it is not set it returns an ErrNotFound error. | ||
// If value decoding fails then an ErrEncoding is returned. | ||
func (i Item[V]) Get(ctx context.Context) (V, error) { | ||
return (Map[noKey, V])(i).Get(ctx, noKey{}) | ||
var toCache bool |
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.
is this boolean really needed?
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.
Its used for case cache is empty when we get value from Item. So we will cache value of 1st get.
closing this as we are upstreaming server modular to main and should add a quick adr on the design |
Description
Closes: #19223
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Add object cache to stf context
get, set, remove of items affects cache
add tests
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...