strongduanmu commented on PR #38707:
URL: https://github.com/apache/shardingsphere/pull/38707#issuecomment-4497521039
Merge Verdict: Mergeable
Reviewed Scope
-
kernel/transaction/core/src/main/java/org/apache/shardingsphere/transaction/savepoint/ConnectionSavepointManager.java
-
proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/PostgreSQLPortalContextRegistry.java
- .codex/skills/review-pr/SKILL.md
Not Reviewed Scope
- GitHub check status, per reviewer instruction.
- Local CI/E2E rerun, because the author/reviewer confirmed they already
passed locally.
- Performance benchmark, because this is a known JDK 8
ConcurrentHashMap#computeIfAbsent optimization pattern.
Review Basis
The runtime changes directly address the intended root cause: avoiding the
JDK 8 ConcurrentHashMap#computeIfAbsent hot-path performance issue when the
cached value already exists.
Both modified runtime paths now follow the expected pattern:
1. Read the cache first with get.
2. Call computeIfAbsent only on cache miss.
3. Return the resolved cached value.
This preserves the original lazy initialization behavior while avoiding
unnecessary computeIfAbsent calls on cache hits.
For ConnectionSavepointManager, the refactoring extracts the savepoint map
lookup into getConnectionSavepoints, keeping the existing behavior of creating
a per-connection savepoint map only when absent.
For PostgreSQLPortalContextRegistry, the portal context lookup keeps the
same per-connection cache semantics and still creates a new PortalContext only
when the connection id has no cached context.
The .codex/skills/review-pr/SKILL.md update is aligned with the PR intent
because it codifies the same review rule for future high-frequency Proxy/JDBC
paths.
Risk Assessment
- Design: acceptable. The change is localized and keeps the existing cache
ownership unchanged.
- Performance: improved for cache-hit paths on JDK 8.
- Compatibility: no public API, SPI, SQL semantics, config, or protocol
behavior changes.
- Regression risk: low. Cache miss behavior remains guarded by
computeIfAbsent, so concurrent initialization semantics are preserved.
- Documentation: no user-facing documentation update required.
Pre-Merge Checks
- Local CI and E2E were reported as passed.
- No extra performance test is required for this known JDK 8 optimization
pattern.
This PR is safe to merge under the reviewed scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
