Perf fixes #1678
Merged
Perf fixes #1678
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- add canonical condition to sql to hit the index
WalkthroughThe changes involve a shift in the method for retrieving the head block within the enclave's logic, moving from a storage-based approach to one that utilizes a block processor. Additionally, there's an update to SQL queries to filter for canonical batches when fetching transaction lists and counts, ensuring that only data associated with canonical batches is considered. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/enclave/enclave.go (2 hunks)
- go/enclave/storage/enclavedb/batch.go (1 hunks)
Additional comments: 2
go/enclave/enclave.go (1)
- 322-328: The change from
storage.FetchHeadBlock()tol1BlockProcessor.GetHead()in theStatusmethod is consistent with the PR's performance objectives. However, ensure that the error handling is robust, especially since the comment on line 327 suggests that failing to fetch the L1 head block might be normal during startup.go/enclave/storage/enclavedb/batch.go (1)
- 47-48: The changes to the SQL queries
queryTxListandqueryTxCountListcorrectly implement the performance improvement by adding thewhere batch.is_canonical=truecondition. This should leverage the [height, is_canonical] index more effectively as intended in the PR objectives. Ensure that the rest of the system expects and correctly handles the now potentially reduced result set due to this new filter.
Comment on lines
1472
to
1476
| var hash common.L1BlockHash | ||
| l1Head, err := e.storage.FetchHeadBlock() | ||
| l1Head, err := e.l1BlockProcessor.GetHead() | ||
| // todo - handle error | ||
| if err == nil { | ||
| hash = l1Head.Hash() |
There was a problem hiding this comment.
The rejectBlockErr method has been updated to use l1BlockProcessor.GetHead() instead of storage.FetchHeadBlock(). It is important to handle the potential error from GetHead() call properly. Currently, there is a todo comment indicating that error handling needs to be addressed. This should be resolved to ensure that errors are handled correctly and do not lead to unexpected behavior.
l1Head, err := e.l1BlockProcessor.GetHead()
- // todo - handle error
+ if err != nil {
+ // Handle the error appropriately
+ }
if err == nil {Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| var hash common.L1BlockHash | |
| l1Head, err := e.storage.FetchHeadBlock() | |
| l1Head, err := e.l1BlockProcessor.GetHead() | |
| // todo - handle error | |
| if err == nil { | |
| hash = l1Head.Hash() | |
| var hash common.L1BlockHash | |
| l1Head, err := e.l1BlockProcessor.GetHead() | |
| if err != nil { | |
| // Handle the error appropriately | |
| } | |
| if err == nil { | |
| hash = l1Head.Hash() |
tudor-malene
added a commit
that referenced
this pull request
Dec 8, 2023
- add canonical condition to sql to hit the index (cherry picked from commit fb50c7b)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this change is needed
to improve performance
What changes were made as part of this PR
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks