fix(#4095): synchronized replaced with lock in Heaps.java#4924
fix(#4095): synchronized replaced with lock in Heaps.java#4924yegor256 merged 1 commit intoobjectionary:masterfrom
Conversation
📝 WalkthroughWalkthroughThe Heaps class synchronization mechanism was refactored to use ReentrantLock instead of synchronized blocks for guarding the blocks map. All memory operations—malloc, size, resize, read, write, and free—now use explicit lock/unlock patterns with try/finally blocks while maintaining the same public API. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eo-runtime/src/main/java/EOorg/EOeolang/Heaps.java (1)
34-47: Consider replacingConcurrentHashMapwithHashMap.Since all access to
blocksis now protected by theReentrantLock, the internal synchronization ofConcurrentHashMapis redundant. A plainHashMapwould provide the same thread-safety guarantees with less overhead.♻️ Proposed simplification
-import java.util.concurrent.ConcurrentHashMap; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock;- `@SuppressWarnings`("PMD.LooseCoupling") - private final ConcurrentHashMap<Integer, byte[]> blocks; + private final Map<Integer, byte[]> blocks;- this.blocks = new ConcurrentHashMap<>(0); + this.blocks = new HashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/java/EOorg/EOeolang/Heaps.java` around lines 34 - 47, The field blocks in class Heaps is using ConcurrentHashMap despite all access being guarded by the ReentrantLock; change the declaration and initialization of blocks from ConcurrentHashMap<Integer, byte[]> to plain HashMap<Integer, byte[]> (update the type on the field and the constructor initialization in Heaps()), remove any unnecessary concurrency imports (ConcurrentHashMap) and keep the existing lock usage in methods that access blocks to preserve thread-safety; ensure any `@SuppressWarnings` remains appropriate or remove it if no longer needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eo-runtime/src/main/java/EOorg/EOeolang/Heaps.java`:
- Around line 34-47: The field blocks in class Heaps is using ConcurrentHashMap
despite all access being guarded by the ReentrantLock; change the declaration
and initialization of blocks from ConcurrentHashMap<Integer, byte[]> to plain
HashMap<Integer, byte[]> (update the type on the field and the constructor
initialization in Heaps()), remove any unnecessary concurrency imports
(ConcurrentHashMap) and keep the existing lock usage in methods that access
blocks to preserve thread-safety; ensure any `@SuppressWarnings` remains
appropriate or remove it if no longer needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0482e23-1f47-43f1-9611-50ed82e62460
📒 Files selected for processing (1)
eo-runtime/src/main/java/EOorg/EOeolang/Heaps.java
|
@yegor256 could you check this PR? |
|
@eshabakhov thanks! |
|
@rultor release, tag is |
|
@rultor status |
|
@eshabakhov Thanks for the contribution! You've earned +16 points following our bonus policy - you received the base 16 points for having reviews, plus additional points for your code contribution size, with appropriate adjustments applied. Please, keep them coming! Your running score is +16; don't forget to check your Zerocracy account too. |
Summary
This PR solves #4905
Changes
synchronizedstatements replaced withlockin Heaps.javaTest
Run the following commands to verify:
Summary by CodeRabbit