-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(api): Optimize memory allocation for the filter interface of JSON-RPC #6495
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
fix(api): Optimize memory allocation for the filter interface of JSON-RPC #6495
Conversation
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpc.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Outdated
Show resolved
Hide resolved
chainbase/src/main/java/org/tron/core/db2/core/SnapshotManager.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Show resolved
Hide resolved
waynercheung
left a comment
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.
LGTM
eth_newFilter interface | public int jsonRpcMaxSubTopics = 1000; | ||
| @Getter | ||
| @Setter | ||
| public int jsonRpcMaxFilterNum = 50000; |
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.
Has this default value been tested, or is there any other reference for it?
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.
Yes. It has been tested for mainnet. But nile should set it as 30000 or smaller.
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpc.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Outdated
Show resolved
Hide resolved
84fa97c to
52a662e
Compare
| continue; | ||
| } | ||
| entry.getValue().getResult().add(ByteArray.toJsonHex(blockFilterCapsule.getBlockHash())); | ||
| String finalBlockHash = blockHash; |
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.
what about moving cache lookup outside the loop to avoid redundant queries.
Since originalBlockHash is constant, querying the cache N times in the
loop is unnecessary. Moving it outside:
- Reduces O(N) cache accesses to O(1)
- Improves performance with identical functionality
- Better error handling with fallback strategy
// Approach A: Optimal - Query the cache once outside the loop (Recommended)
final String originalBlockHash = ByteArray.toJsonHex(blockFilterCapsule.getBlockHash());
// Query the cache only once
String cachedBlockHash;
try {
cachedBlockHash = blockHashCache.get(originalBlockHash, () -> originalBlockHash);
} catch (ExecutionException e) {
logger.error("Getting/loading blockHash from cache failed", e);
cachedBlockHash = originalBlockHash; // Fallback: use the original value
}
// Use the cached result directly inside the loop
while (it.hasNext()) {
Entry<String, BlockFilterAndResult> entry = it.next();
if (entry.getValue().isExpire()) {
it.remove();
continue;
}
entry.getValue().getResult().add(cachedBlockHash);
}
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.
It might put unnecessary blockHash into blockHashCache if filter map is empty. But it's a good suggestion
…newFilter, eth_newBlockFilter
52a662e to
1bc6bdd
Compare
waynercheung
left a comment
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.
LGTM
What does this PR do?
eth_newFilter,eth_newBlockFilternode.jsonrpc.maxBlockFilterNumWhy are these changes required?
This PR has been tested by:
Follow up
Extra details