(#4884) Replaced synchronized with ReentrantLock in PhOnce#4914
(#4884) Replaced synchronized with ReentrantLock in PhOnce#4914AlexNetTie wants to merge 5 commits intoobjectionary:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eo-runtime/src/main/java/org/eolang/PhOnce.java (1)
45-53: Consider a fast path to avoid locking after initialization.Line 45 currently acquires the lock on every access, even when
refis already initialized. A pre-check + in-lock recheck keeps correctness while reducing contention on hot paths.♻️ Suggested refactor
this.object = () -> { - lock.lock(); - try { - if (this.ref.get() == null) { - this.ref.set(obj.get()); - } - return this.ref.get(); - } finally { - lock.unlock(); - } + Phi cached = this.ref.get(); + if (cached == null) { + this.lock.lock(); + try { + cached = this.ref.get(); + if (cached == null) { + cached = obj.get(); + this.ref.set(cached); + } + } finally { + this.lock.unlock(); + } + } + return cached; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/java/org/eolang/PhOnce.java` around lines 45 - 53, PhOnce currently always acquires the lock on every access; add a fast-path null check to return this.ref.get() immediately when already initialized, then perform the existing lock + in-lock recheck to set ref only if still null (i.e., in the method containing lock.lock()/lock.unlock() replace the unconditional lock with: if (this.ref.get() != null) return this.ref.get(); lock.lock(); try { if (this.ref.get() == null) this.ref.set(obj.get()); return this.ref.get(); } finally { lock.unlock(); }). Ensure you reference the PhOnce class, the ref field, the lock and the supplier parameter (obj) so the semantics remain identical while reducing contention.
🤖 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/org/eolang/PhOnce.java`:
- Around line 45-53: PhOnce currently always acquires the lock on every access;
add a fast-path null check to return this.ref.get() immediately when already
initialized, then perform the existing lock + in-lock recheck to set ref only if
still null (i.e., in the method containing lock.lock()/lock.unlock() replace the
unconditional lock with: if (this.ref.get() != null) return this.ref.get();
lock.lock(); try { if (this.ref.get() == null) this.ref.set(obj.get()); return
this.ref.get(); } finally { lock.unlock(); }). Ensure you reference the PhOnce
class, the ref field, the lock and the supplier parameter (obj) so the semantics
remain identical while reducing contention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e139a486-0241-4a2d-80d6-b677eef3fafb
📒 Files selected for processing (1)
eo-runtime/src/main/java/org/eolang/PhOnce.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eo-runtime/src/main/java/org/eolang/PhOnce.java`:
- Around line 44-58: The Supplier<Phi> lambda assigned to this.object in
PhOnce.java currently never returns a Phi; inside the supplier (which reads
cached via this.ref, locks this.lock, calls obj.get(), sets this.ref) add a
final "return cached;" just before the lambda ends so the Supplier<Phi> returns
the computed Phi value (ensure the returned variable is the same cached used
after obj.get()/this.ref.set()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6e91ffe-08c5-4536-b1c3-0574d52e0f3e
📒 Files selected for processing (1)
eo-runtime/src/main/java/org/eolang/PhOnce.java
…ocking The previous implementation didn't store the result in a local variable, causing unnecessary lock acquisition after initialization. Now using standard DCL pattern with local cached variable for better performance.
🚀 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
|
What does this PR do?
Replaces
synchronizedblock withReentrantLockinPhOnceconstructor to avoid blocking the whole object during lazy initialization and to comply with qulice rules.Related issues
Changes
ReentrantLockfieldsynchronized (this.ref)withlock.lock()/lock.unlock()in try-finally block@todopuzzleHow to test
Run
mvn clean install -pl eo-runtime -amto verify the module compiles. The change is internal and should not affect existing behavior.Summary by CodeRabbit