perf(txmetacache): optimize memory usage by 4.62 GB#475
Open
freemans13 wants to merge 5 commits intobsv-blockchain:mainfrom
Open
perf(txmetacache): optimize memory usage by 4.62 GB#475freemans13 wants to merge 5 commits intobsv-blockchain:mainfrom
freemans13 wants to merge 5 commits intobsv-blockchain:mainfrom
Conversation
…y length prefix
This commit implements two memory optimizations for the txmetacache:
1. **Remove dead height-based expiry code (3.08 GB savings)**
- Removed appendHeightToValue function that appended 4-byte height suffix
- The height was never read or checked, making it pure overhead
- Removed noOfBlocksToKeepInTxMetaCache field and related configuration
- Removed hitOldTx metric (never incremented)
- Updated misleading comments about height-based expiry
- Production always reaches capacity before expiry timeout
2. **Optimize key encoding by removing key length prefix (1.54 GB savings)**
- Transaction hashes are always 32 bytes, so key length prefix is redundant
- Changed encoding format from [2B:keyLen][2B:valLen][key][val]
to [2B:valLen][32B:key][val]
- Updated all three bucket implementations:
* bucketUnallocated.Set/Get
* bucketPreallocated.Set/Get
* bucketTrimmed.Set/Get
- Keys must now be exactly 32 bytes (enforced with validation)
**Memory savings at 770M transactions:**
- Removed height suffix: 4 bytes × 770M = 3.08 GB
- Removed key length: 2 bytes × 770M = 1.54 GB
- **Total: 4.62 GB saved** (from ~95GB → ~90GB)
**Testing:**
- Updated test cases to use 32-byte keys (via makeKey32 helper)
- Main test suites passing (TestImprovedCache_SetAndGet, etc.)
- Some specialized tests need updates for 32-byte key requirement
**Breaking changes:**
- Cache now only accepts 32-byte keys (transaction hashes)
- Old cached data with different encoding will be invalid (cache will be rebuilt)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Updated TestImprovedCache_TestSetMultiWithExpectedMisses capacity calculation - Fixed all tests to use makeKey32() helper for 32-byte keys - Updated TestImprovedCache_LargeKeys to test key size validation - Fixed TestImprovedCache_EdgeCases to expect empty key rejection - Fixed Test_txMetaCache_MultiOperations for removed height encoding All ImprovedCache and txMetaCache tests now passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: The boundary check issue from the previous review has been fixed - all three bucket implementations now correctly use > instead of >= when validating idx+keyLen+valLen > ChunkSize. Minor documentation cleanup needed:
These are documentation-only issues that do not affect functionality. History:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
stores/txmetacache/improved_cache.go
Outdated
| valLen := (uint64(vLenBuf[0]) << 8) | uint64(vLenBuf[1]) | ||
| idx += 2 | ||
|
|
||
| if idx+keyLen+valLen >= ChunkSize { |
Contributor
There was a problem hiding this comment.
The boundary check logic has an off-by-one error.
Issue: After incrementing idx by 2, this check validates whether we can read keyLen+valLen bytes starting at idx. However, using >= is too strict.
Analysis:
- Valid chunk indices: 0 to ChunkSize-1
- After reading 2-byte value length, idx points to the key
- We need to read bytes from idx to idx+keyLen+valLen-1
- The last byte at idx+keyLen+valLen-1 must be < ChunkSize
- Therefore: idx+keyLen+valLen-1 < ChunkSize, which means idx+keyLen+valLen ≤ ChunkSize
Example (ChunkSize=100):
- If idx=65, keyLen=32, valLen=3, then idx+keyLen+valLen=100
- Current code: 100 >= 100 → rejects (incorrect)
- Should read bytes 65-96 (key) and 97-99 (value), which are all valid
Fix: Change >= to > on lines 917, 1303, and 1608:
if idx+keyLen+valLen > ChunkSize {This affects all three bucket implementations.
- Fix gci import ordering in improved_cache.go and improved_cache_test.go - Add nolint:gosec comments for vLenBuf[1] accesses (false positives on fixed-size array) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
This PR implements two memory optimizations for the txmetacache, saving 4.62 GB at 770M transactions:
1. Remove Dead Height-Based Expiry Code (3.08 GB saved)
appendHeightToValue()function that added 4-byte height suffixnoOfBlocksToKeepInTxMetaCachefield andhitOldTxmetric2. Optimize Key Encoding (1.54 GB saved)
[2B:keyLen][2B:valLen][key][val]→[2B:valLen][32B:key][val]Memory Impact (at 770M transactions)
Testing
Breaking Changes
Performance Impact
🤖 Generated with Claude Code