perf(node-api): Add caching of query contexts to backend #1819
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates integrate a caching mechanism into the Changes
Sequence Diagram(s)sequenceDiagram
participant Backend
participant QueryCache
participant Node
Backend->>QueryCache: GetQueryContext(slot)
alt Cache Hit
QueryCache-->>Backend: Return existing context
else Cache Miss
Backend->>Node: CreateQueryContext(slot)
Node-->>Backend: Return new context
Backend->>QueryCache: AddQueryContext(slot, new context)
end
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 Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1819 +/- ##
=======================================
Coverage 24.97% 24.98%
=======================================
Files 326 330 +4
Lines 14184 14227 +43
Branches 19 19
=======================================
+ Hits 3543 3555 +12
- Misses 10511 10542 +31
Partials 130 130
|
Signed-off-by: Cal Bera <[email protected]>
Signed-off-by: Cal Bera <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (4)
- mod/node-api/backend/backend.go (4 hunks)
- mod/node-api/backend/cache/cache.go (1 hunks)
- mod/node-api/backend/cache/cache_test.go (1 hunks)
- mod/node-api/backend/cache/config.go (1 hunks)
Additional context used
GitHub Check: nilaway
mod/node-api/backend/backend.go
[failure] 189-189:
b.sb.StateFromContext(queryCtx).Copy undefined (type BeaconStateT has no field or method Copy)
Additional comments not posted (25)
mod/node-api/backend/cache/config.go (6)
1-19: File License and Copyright NoticeThe file contains a detailed license notice, which is important for legal compliance. Ensure that this notice is consistent across all files in the repository.
21-22: Package DeclarationThe package is correctly declared as
cache, which aligns with the file's purpose.
23-23: Import StatementThe
timepackage is imported, which is necessary for thetime.Durationtype used in the configuration.
25-28: Constants DefinitionThe default values for
QueryContextSizeandQueryContextTTLare defined. These values are sensible defaults for a cache configuration.
30-36: Config Struct DefinitionThe
Configstruct is well-defined with appropriate fields for cache size and TTL. The use ofmapstructuretags indicates this struct can be easily used with configuration libraries.
38-44: DefaultConfig FunctionThe
DefaultConfigfunction returns aConfigstruct with default values. This is a good practice to provide a standard configuration.mod/node-api/backend/cache/cache.go (8)
1-19: File License and Copyright NoticeThe file contains a detailed license notice, which is important for legal compliance. Ensure that this notice is consistent across all files in the repository.
21-22: Package DeclarationThe package is correctly declared as
cache, which aligns with the file's purpose.
23-28: Import StatementsThe necessary packages are imported, including the
golang-lrupackage for the LRU cache implementation and a custommathpackage.
30-35: QueryCache Struct DefinitionThe
QueryCachestruct is defined with a single field for the LRU cache. The use of generics in the LRU cache is appropriate for type safety.
37-45: NewQueryCache ConstructorThe
NewQueryCachefunction initializes aQueryCachewith the provided configuration. The use of the LRU cache constructor is correct, and the function properly handles the configuration parameters.
48-52: NewQueryCacheWithDefaultConfig ConstructorThis function provides a convenient way to create a
QueryCachewith default settings. It correctly calls theNewQueryCachefunction with the default configuration.
54-57: GetQueryContext MethodThe
GetQueryContextmethod retrieves a context from the cache. The method is simple and correctly returns both the context and a boolean indicating whether it was found.
59-62: AddQueryContext MethodThe
AddQueryContextmethod adds a context to the cache. The method is straightforward and correctly uses the LRU cache'sAddmethod.mod/node-api/backend/cache/cache_test.go (8)
1-19: File License and Copyright NoticeThe file contains a detailed license notice, which is important for legal compliance. Ensure that this notice is consistent across all files in the repository.
21-22: Package DeclarationThe package is correctly declared as
cache_test, which is a common convention for testing packages in Go.
23-31: Import StatementsThe necessary packages are imported, including the
testingpackage for writing tests, thecachepackage being tested, and therequirepackage fromtestifyfor assertions.
33-38: TestQueryCache FunctionThe
TestQueryCachefunction sets up the test environment, including creating a configuration and initializing theQueryCache. This setup is clear and appropriate.
42-46: Test Case: Get from Empty CacheThis test case checks the behavior of the
GetQueryContextmethod when the cache is empty. The assertions correctly verify that the context is not found and is nil.
48-55: Test Case: Set and GetThis test case verifies that a context can be added to the cache and subsequently retrieved. The assertions ensure that the context is correctly stored and retrieved.
57-71: Test Case: Overwrite ExistingThis test case checks that an existing context in the cache can be overwritten. The assertions verify that the new context replaces the old one.
73-80: Test Case: Prune and Verify DeletionThis test case ensures that the cache correctly prunes old entries when the cache size limit is reached. The assertions verify that the oldest entry is removed.
mod/node-api/backend/backend.go (3)
119-122: LGTM! Initialization of the cache field is correct.The
Newfunction correctly initializes thecachefield usingcache.NewQueryCacheWithDefaultConfig().
Line range hint
153-167:
LGTM! Dependency onstateFromSlotRaw.The
stateFromSlotfunction looks correct but relies onstateFromSlotRaw, which has a potential issue with theCopymethod.
175-189: Potential issue with theCopymethod.The static analysis tool indicates that
b.sb.StateFromContext(queryCtx).Copyis undefined. Ensure thatBeaconStateThas aCopymethod or replace it with an appropriate method to create a copy of the state.- st = b.sb.StateFromContext(queryCtx).Copy() + st = b.sb.StateFromContext(queryCtx).DeepCopy() // Assuming DeepCopy is the correct methodTools
GitHub Check: nilaway
[failure] 189-189:
b.sb.StateFromContext(queryCtx).Copy undefined (type BeaconStateT has no field or method Copy)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- mod/node-api/backend/backend.go (6 hunks)
- mod/node-api/backend/types.go (1 hunks)
Additional comments not posted (6)
mod/node-api/backend/types.go (2)
68-68: LGTM!The addition of the type parameter
Tto theBeaconStateinterface enhances its flexibility and allows it to work with any specified type.
74-75: LGTM!The addition of the
Copy()method to theBeaconStateinterface is a valuable enhancement for maintaining state consistency in concurrent or distributed systems.mod/node-api/backend/backend.go (4)
26-26: LGTM!The import of the
cachepackage is necessary for implementing the caching mechanism.
68-68: LGTM!The addition of the
cachefield to theBackendstruct enables caching of query contexts, improving performance by reducing redundant context creation.
122-122: LGTM!The initialization of the
cachefield in theNewconstructor function ensures that each instance ofBackendis equipped with caching capability.
175-189: LGTM!The modification of the
stateFromSlotRawmethod to utilize thecachefield for query context management enhances performance by reducing redundant context creation. Copying the state ensures the integrity of the original state.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- mod/node-api/backend/cache/cache.go (1 hunks)
- mod/node-api/backend/cache/cache_test.go (1 hunks)
- mod/node-api/backend/cache/config.go (1 hunks)
Additional comments not posted (14)
mod/node-api/backend/cache/config.go (5)
1-19: File header looks good.The license information is correct and complete.
21-23: Package declaration and imports look good.The package name
cacheis appropriate, and thetimeimport is necessary for TTL configuration.
25-31: Constants for default cache size and TTL look good.The default values are reasonable and the comments provide clear explanations.
33-39: Config struct looks good.The struct is well-defined and the tags are correct for configuration parsing.
41-47: DefaultConfig function looks good.The function correctly initializes the
Configstruct with default values.mod/node-api/backend/cache/cache.go (6)
1-19: File header looks good.The license information is correct and complete.
21-28: Package declaration and imports look good.The package name
cacheis appropriate, and the imports are necessary for the cache implementation.
30-35: QueryCache struct looks good.The struct is well-defined and the comments provide clear explanations.
37-45: NewQueryCache function looks good.The function correctly initializes the
QueryCachewith the provided configuration.
48-52: NewQueryCacheWithDefaultConfig function looks good.The function correctly initializes the
QueryCachewith the default configuration.
54-64: Cache operations methods look good.The methods are well-defined and provide necessary cache operations.
mod/node-api/backend/cache/cache_test.go (3)
1-19: File header looks good.The license information is correct and complete.
21-31: Package declaration and imports look good.The package name
cache_testis appropriate, and the imports are necessary for the tests.
33-84: TestQueryCache function looks good.The function is well-structured and covers different scenarios for cache operations.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
mod/node-api/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- mod/node-api/go.mod (1 hunks)
Additional comments not posted (1)
mod/node-api/go.mod (1)
13-13: Dependency addition approved.The addition of
github.com/hashicorp/golang-lru/v2 v2.0.7is appropriate for implementing an LRU cache. Ensure that this version is the latest stable release and verify if there are any known issues with it.Verification successful
Dependency addition approved.
The addition of
github.com/hashicorp/golang-lru/v2 v2.0.7is appropriate for implementing an LRU cache. This version is the latest stable release.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the added dependency version is the latest stable release. # Test: Check the latest version of the dependency. Expect: Version v2.0.7 or higher. go list -m -versions github.com/hashicorp/golang-lru/v2Length of output: 127
Script:
#!/bin/bash # Description: Verify if the added dependency version is the latest stable release by querying the GitHub API. # Test: Check the latest release version of the dependency on GitHub. Expect: Version v2.0.7 or higher. curl -s https://api.github.com/repos/hashicorp/golang-lru/releases/latest | jq -r .tag_nameLength of output: 100
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
mod/config/go.sumis excluded by!**/*.sum
Files selected for processing (12)
- mod/cli/pkg/flags/flags.go (2 hunks)
- mod/config/config.go (3 hunks)
- mod/config/go.mod (3 hunks)
- mod/config/pkg/template/template.go (1 hunks)
- mod/node-api/backend/backend.go (7 hunks)
- mod/node-api/backend/cache/cache.go (1 hunks)
- mod/node-api/backend/cache/cache_test.go (1 hunks)
- mod/node-api/backend/cache/config.go (1 hunks)
- mod/node-api/backend/config.go (1 hunks)
- mod/node-api/config.go (1 hunks)
- mod/node-core/pkg/components/api.go (3 hunks)
- testing/files/entrypoint.sh (1 hunks)
Additional comments not posted (39)
mod/node-api/backend/config.go (3)
21-23: LGTM!The package declaration and import statement are correct.
25-27: LGTM!The
Configstruct definition is correct and aligns with the objective of configuring the cache.
29-33: LGTM!The
DefaultConfigfunction correctly initializes theConfigstruct with default values.mod/node-api/config.go (3)
21-26: LGTM!The package declaration and import statements are correct.
28-31: LGTM!The
Configstruct definition is correct and aligns with the objective of configuring the node API.
33-38: LGTM!The
DefaultConfigfunction correctly initializes theConfigstruct with default values.mod/node-api/backend/cache/config.go (3)
21-23: LGTM!The package declaration and import statement are correct.
25-31: LGTM!The constants for default values are correctly defined and documented.
33-47: LGTM!The
Configstruct definition andDefaultConfigfunction are correct and align with the objective of configuring the cache.mod/node-api/backend/cache/cache.go (4)
30-35: LGTM! The struct definition looks correct.The
QueryCachestruct is well-defined, using an LRU cache for query contexts.
37-45: LGTM! The function correctly initializes the cache.The
NewQueryCachefunction properly initializes the LRU cache with the provided configuration.
48-51: LGTM! The function correctly retrieves the context.The
GetQueryContextfunction correctly retrieves the query context for a given slot from the cache.
53-58: LGTM! The function correctly adds the context.The
AddQueryContextfunction correctly adds the query context to the cache for a given slot.mod/node-core/pkg/components/api.go (4)
Line range hint
44-49:
LGTM! The struct definition looks correct.The addition of the
Configfield to theNodeAPIBackendInputstruct enhances flexibility and configurability.
Line range hint
51-75:
LGTM! The function correctly uses the new configuration field.The
ProvideNodeAPIBackendfunction correctly uses thein.Config.NodeAPI.Backendfield, improving specificity and clarity.
Line range hint
77-83:
LGTM! The struct definition looks correct.The addition of the
Configfield to theNodeAPIServerInputstruct enhances flexibility and configurability.
Line range hint
85-98:
LGTM! The function correctly uses the new configuration field.The
ProvideNodeAPIServerfunction correctly uses thein.Config.NodeAPI.Serverfield, improving specificity and clarity.mod/node-api/backend/cache/cache_test.go (5)
33-39: LGTM! The initialization of theQueryCacheis correct.The
QueryCacheis correctly initialized with the provided configuration.
43-47: LGTM! The sub-test correctly validates the behavior.The sub-test
Get from empty cachecorrectly validates that getting a context from an empty cache returns false and nil.
49-56: LGTM! The sub-test correctly validates the behavior.The sub-test
Set and Getcorrectly validates that setting and then getting a context returns the correct value.
58-74: LGTM! The sub-test correctly validates the behavior.The sub-test
Overwrite existingcorrectly validates that overwriting an existing context updates the value correctly.
76-83: LGTM! The sub-test correctly validates the behavior.The sub-test
Prune and verify deletioncorrectly validates that the cache prunes old entries and verifies their deletion.mod/config/config.go (2)
32-32: LGTM!The import statement for the
NodeAPImodule has been renamed fromservertonodeapi, reflecting a shift in how theNodeAPIconfiguration is accessed.
52-52: LGTM!The
NodeAPIfield type in theConfigstruct and theDefaultConfigfunction has been updated fromserver.Configtonodeapi.Config, reflecting a semantic shift in the underlying configuration structure for theNodeAPI.Also applies to: 71-71
mod/config/pkg/template/template.go (2)
98-98: LGTM!The parameters
enabled,address, andlogginghave been updated to reference the new structure[beacon-kit.node-api.server], enhancing the clarity and organization of the configuration.Also applies to: 100-106
108-113: LGTM!The new section
[beacon-kit.node-api.backend]includes parameterssizeandquery-context-ttlfor backend settings, likely aimed at improving performance and resource management.mod/cli/pkg/flags/flags.go (6)
73-73: LGTM!The introduction of
nodeAPIRootimproves the organization and modularity of the configuration settings.
74-74: LGTM!The introduction of
nodeAPIServerRootenhances clarity by separating server-related configurations from backend configurations.
75-75: LGTM!The introduction of
nodeAPIBackendRootenhances clarity by separating backend-related configurations from server configurations.
79-79: LGTM!The introduction of
NodeAPICacheSizeallows for configuration of cache-related parameters, improving performance and efficiency.
80-80: LGTM!The introduction of
NodeAPICacheQueryContextTTLallows for configuration of cache-related parameters, improving performance and efficiency.
157-179: LGTM!The updates to
AddBeaconKitFlagsensure that the new configuration paths are correctly referenced, improving the organization and maintainability of the code.mod/node-api/backend/backend.go (4)
26-26: LGTM!The import statement for
cacheis necessary for integrating the caching mechanism.
68-68: LGTM!The addition of the
cachefield enables the caching of query contexts, improving performance by reducing the overhead of creating new contexts for the same slots.
123-123: LGTM!The initialization of the
cachefield ensures that each instance ofBackendis equipped with caching capabilities tailored to the provided configuration.
176-190: LGTM!The updates to
stateFromSlotRaweffectively utilize the cache for query context management, improving performance and ensuring the integrity of the cached data by copying the state.mod/config/go.mod (3)
69-69: LGTM!The addition of
github.com/golang-jwt/jwt v3.2.2+incompatibleas an indirect dependency may be necessary for compatibility with other components or for specific features not present in the newer version.
116-116: LGTM!The addition of
github.com/stretchr/objx v0.5.2as an indirect dependency suggests a need for enhanced object manipulation capabilities within the application.
132-132: LGTM!The addition of
golang.org/x/time v0.5.0as an indirect dependency indicates a need for enhanced time functionalities, possibly to improve time management or scheduling features in the application.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- mod/cli/pkg/flags/flags.go (2 hunks)
- mod/config/pkg/template/template.go (1 hunks)
Additional comments not posted (5)
mod/config/pkg/template/template.go (2)
98-107: LGTM: Node API server configuration restructuring.The new section
[beacon-kit.node-api.server]clearly separates server-related settings, improving configuration management.
108-113: LGTM: Backend cache configuration addition.The new section
[beacon-kit.node-api.backend.cache]adds settings for cache size and TTL, aligning with the caching implementation objectives.mod/cli/pkg/flags/flags.go (3)
73-81: LGTM: Node API configuration constants restructuring.The new constants
nodeAPIServerRoot,nodeAPIBackendRoot, andnodeAPICacheRootimprove the organization of Node API settings.
158-168: LGTM: Node API server flag updates.The updated flag definitions correctly reference the new server configuration paths, enhancing clarity and maintainability.
171-180: LGTM: Backend cache flag additions.The new flag definitions for cache size and TTL align with the caching implementation objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- mod/consensus-types/pkg/types/payload_header_test.go (2 hunks)
Additional comments not posted (2)
mod/consensus-types/pkg/types/payload_header_test.go (2)
42-45: LGTM! But verify the appropriateness of the new initialization values.The changes ensure that the
ExecutionPayloadHeaderis instantiated with specific non-empty values. Verify if these values are appropriate for all use cases.
71-74: LGTM!The changes align the test assertions with the updated initialization values, ensuring that the tests correctly validate the new default state of the
ExecutionPayloadHeader.
| ) | ||
|
|
||
| // QueryCache is a cache for query contexts. | ||
| type QueryCache struct { |
There was a problem hiding this comment.
I get the motivation here, but I am ensure if this is actually doing anything, you'll have to do some benchmarks / traces
There was a problem hiding this comment.
yea we don't have traces set up anywhere. was thinking to use this lib https://github.com/open-telemetry/opentelemetry-go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- mod/node-api/backend/backend.go (7 hunks)
Additional comments not posted (5)
mod/node-api/backend/backend.go (5)
26-26: Import statement forcachelooks good.The import statement for
cacheis necessary for the new caching mechanism.
68-68: Addition ofcachefield inBackendstruct looks good.The new field
cacheis necessary for the caching mechanism.
123-123: Initialization ofcachefield inNewfunction looks good.The
cachefield is correctly initialized usingcache.NewQueryCache(cfg.Cache).
181-190: Changes tostateFromSlotRawfunction look good.The function correctly leverages the new
cachefield for query context management.
190-190: State copying instateFromSlotRawfunction looks good.The state is correctly copied to prevent unintended mutations.
|
postponing this till we have tracers enabled and live trace data #1849 |
calbera
changed the title
LRU cache the query contexts (to optimize for 1 endpoint handler calling backend multiple times, should only incur 1 query context)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation