further improvement of Lazy.get() #1571
Conversation
Current coverage is 96.98% (diff: 100%)@@ master #1571 diff @@
==========================================
Files 88 88
Lines 10642 10642
Methods 0 0
Messages 0 0
Branches 1813 1813
==========================================
+ Hits 10320 10321 +1
Misses 179 179
+ Partials 143 142 -1
|
| @@ -133,12 +135,13 @@ private Lazy(Supplier<? extends T> supplier) { | |||
| */ | |||
| @Override | |||
| public T get() { | |||
| // using a local var speeds up the double-check idiom by 25%, see Effective Java, Item 71 | |||
| Supplier<? extends T> tmp = supplier; | |||
There was a problem hiding this comment.
I strongly think we should base speedups on JMH benchmarks, our intuition is most often wrong about these ...
There was a problem hiding this comment.
Will create a (local) benchmark and post the results...
| Supplier<? extends T> tmp = supplier; | ||
| if (tmp != null) { | ||
| synchronized (this) { | ||
| tmp = supplier; | ||
| if (tmp != null) { | ||
| value = supplier.get(); | ||
| value = tmp.get(); |
There was a problem hiding this comment.
could you please rename tmp to something meaningful?
There was a problem hiding this comment.
Reads of volatile variables need to access the main memory to ensure the value is actual. tmp could be named cachedSupplier or localSupplier.
Because we return value instead of supplier, we do not need to cache the supplier before entering the synchronized block.
Within the synchronized block we have two reads (1. not-null check, 2. get() call). So a changed supplier saves one main-memory read.
The optimized code then looks like this:
public T get() {
if (this.supplier != null) {
synchronized (this) {
// only one volatile read from main-memory
final Supplier<? extends T> supplier = this.supplier;
if (supplier != null) {
value = supplier.get();
this.supplier = null; // free mem
}
}
}
return value;
}| @@ -133,12 +135,13 @@ private Lazy(Supplier<? extends T> supplier) { | |||
| */ | |||
| @Override | |||
| public T get() { | |||
| // using a local var speeds up the double-check idiom by 25%, see Effective Java, Item 71 | |||
There was a problem hiding this comment.
this comment if for review only, I don't think it should stay in the code.
25%, based on what?
There was a problem hiding this comment.
I agree that we should remove the speed-up comment.
However, it is hard to recognize design patterns/idioms when looking at code. Code is just a textual projection of abstract ideas (very much like 3D to 2D/Screen projection).
I want a comment for developers why we added a local var instead of using the well-known classical double-check idiom.
|
Aargh, sorry @paplorinc. I've overseen the comments. Will apply your comments soon and send a new PR. |
thx @jbgi for the hint!