Modify the cached object for lazy getter

[Nathan Toone]

I am using @Getter(lazy=true), and would like to modify the underlying value...can I also use @Setter (and will it just "know" what value to modify)?  Also, is there a way to clear the "generated" flag (i.e. $lombok$lazy1i) so that the next time the getter is called, it re-evaluates the expensive function?

[Reinier Zwitserloot]

There may be ways, but messing with the $ variables in any way is not supported; we may change how lombok generates the internals to make lazy work at any time. Basically, if you need to either force recalculation or change the value, @Getter(lazy=true) is not for you.

[Reinier Zwitserloot]

We _have_ carefully reviewed that code. We're synchronizing on a private field which in general ought to tell findbugs that it should not be so hasty to call this a bug.

Our code works like this. assume 'FT' is the actual type of the field, so, 'String' in the case of @Getter(lazy=true) String foo = "Hello, World!";

First, we turn the foo field into: AtomicReference<AtomicReference<String>> foo = new AtomicReference(null);

then the getter becomes:

public T getFoo() {

AtomicReference<FT> value = this.fieldName.get();

if (value == null) {

synchronized (this.fieldName) {

value = this.fieldName.get();

if (value == null) { 

final ValueType actualValue = "Hello, World!";

value = AtomicReference<ValueType>(actualValue);

this.fieldName.set(value);

}

}

}

return value.get();

}

Note that the synchronized block is perfectly valid here; it's a trick to get double-checked locking working properly. We also doubly nest AtomicReference so that we can tell the difference between uninitialized (= null) and initialized to null (= new AtomicReference<FT>(null)). The outer atomicreference is so that we have something to lock on.

As this field is at least in theory not accessible by anyone else (well, it's still there for your own class to mess with, but the general idea of @Getter(lazy=true) is of course that you read this thing out via the getter and not directly), we presume nobody is going to mess with it.

On Tuesday, May 21, 2013 3:58:35 PM UTC+2, Jos Claessens wrote:

For your information: FindBugs notes that the generated code for lazy getters may not be correct:

Bug: Synchronization performed on java.util.concurrent.atomic.AtomicReference in XXXXXX (place with @Getter(lazy = true)).

This method performs synchronization an object that is an instance of a class from the java.util.concurrent package (or its subclasses). Instances of these classes have their own concurrency control mechanisms that are orthogonal to the synchronization provided by the Java keyword synchronized. For example, synchronizing on an AtomicBoolean will not prevent other threads from modifying the AtomicBoolean.

Such code may be correct, but should be carefully reviewed and documented, and may confuse people who have to maintain the code at a later date.

Confidence: Normal, Rank: Scary (8)
Pattern: JLM_JSR166_UTILCONCURRENT_MONITORENTER
Type: JLM, Category: MT_CORRECTNESS (Multithreaded correctness)

Maybe you could 'carefully review' the lazy getter's code :)

The problem might just be that the lazy getters make use of the AtomicReference; according to Roel Spilker (http://stackoverflow.com/questions/12160250/lombok-lazy-getter-for-mutable-collections/12160664#12160664) that one will probably be going anyway. I don't know whether the reason for that is this bug, but it might just as well be.


Op woensdag 30 mei 2012 11:59:51 UTC+2 schreef Reinier Zwitserloot het volgende:

[Martin Grajcar]

I'd bet it's correct, I'm just curious if it could be faster. I guess the field needs to be final (otherwise you could use null for uninitialized and AtomicRefererence(value) otherwise. Would n't it work?

There's a "standard hack" way of using some private object for the uninitialized value. Could something like this work?

private final AtomicReference<Object> foo = new AtomicReference<Object>(); 

{

foo.set(foo);

}

@SuppressWarnings("unchecked")

public T getFoo() {

if (foo.get() == foo) {

synchronized (foo) {

if (foo.get() == foo) {

foo.set("my hard to compute value");

}

}

}

return (T) foo;

}

I'm not proposing it... just an exercise. :)

[Reinier Zwitserloot]

We can't just do: private AtomicReference<FT> fieldName = null; because then we wouldn't have anything to lock on. We could make another field to lock on (private final Object $LAZY_LOCKER = new Object(); or private final Object $LAZY_LOCKER_FIELDNAME = new Object(); or some such), but now we're making more objects. 

Your second option, of making the atomic reference reference back to itself to indicate the untinitialized value, DOES work I think. Awesome trick! It would end in return (T) foo.get();, but other than that, I believe your sample is pretty much spot on.

Created an issue for this:

https://code.google.com/p/projectlombok/issues/detail?id=535

 --Reinier Zwitserloot

--
You received this message because you are subscribed to the Google Groups "Project Lombok" group.
To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Martin Grajcar]

It seems to be no big win. except when cache gets tight. But there's another possibility, which seems to be much faster according to my benchmark: Use two fields, the auxiliary one containing a `new Object[0]` for both synchronization and the initial value. The main field needs to be volatile and contains the value directly. I'm not sure now if it's safe.

Btw., the doc shows three fields:

http://projectlombok.org/features/GetterLazy.html

[Reinier Zwitserloot]

The docs list the ideal version as designed by Brian Goetz on the back of a napkin at Devoxx. Because adding fields is actually rather convoluted in eclipse that isn't _actually_ what happens right now. I'm guessing the single self-referencing AtomicReference is similar to what the feature page says in memory footprint and performance and the like.

Could you post the exact code you're benchmarking that's faster? We'll triple-check it for multi-core correctness.

[Tumi]

Hi, I know this is an old thread but I was going to fill a new one, and found this one related.

I strongly suggest changing the current implementation of @Getter(lazy=true) for just injecting the volatile modifier, no longer private final needed, DCL on the volatile, and processor error if the field is marked as final (or at least warning and keep the default direct assigment to that final field). I propose something like:

@Getter(lazy=true) double[] cached = expensive(arg1, arg2, arg3...);

Would generate:

volatile double[] cached = expensive(arg1, arg2, arg3...);

public double[] getCached() {
    double[] cached$localref = this.cached;
    if (cached$localref==null) {
        synchronized(this) { // DLC on volatile variable
            if (this.cached==null) {
                this.cached = cached$localref = expensive(arg1, arg2, arg3);
            }
        }
    }
    return cached$localref;
}

Or maybe a more sofisticated version with more memory footprint but as scalable as currently:

volatile double[] cached = expensive(arg1, arg2, arg3...);
private final Object cached$lazy$lock = new Object();

public double[] getCached() {
    double[] cached$localref = this.cached;
    if (cached$localref==null) {
        synchronized(cached$lazy$lock) {
            if (this.cached==null) { // DLC on volatile variable
                this.cached = cached$localref = expensive(arg1, arg2, arg3);
            }
        }
    }
    return cached$localref;
}

But there may be something that I'm missing about why the AtomicReference was decided to be used?

This alternative implementation would be some cents faster, and more important, more intuitive to the developer as the type is not changed, setter option become available, and any introspection/reflection framework on class fields will report the expected type for lazies.

Cheers!

[Reinier Zwitserloot]

Your proposed implementation will always call the extensive operation, every time the getter is called, if the expensive operation returns null. Also, your proposal creates a second field (for the lock). Our current implementation works even if "expensive" returns null, and has a unique lock per lazy getter without adding extra fields.

[Martin Grajcar]

I can't see why the type of the field should matter as IMHO the point of the lazy getter is to use it for each access.

The case of the expensive operation returning null could be handled easily via the lock field:

private volatile double[] cached;

private final boolean[] cached$lazy$lock = new boolean[1];

public double[] getCached() {
    double[] cached$localref = this.cached;

    if (cached$localref==null && !cached$lazy$lock[0]) {

        synchronized(cached$lazy$lock) {
            if (this.cached==null && !cached$lazy$lock[0]) {

                this.cached = cached$localref = expensive(arg1, arg2, arg3);

               cached$lazy$lock[0] = true;
            }
        }
    }
    return cached$localref;
}

I just can't see the advantage of this solution.

[Roel Spilker]

The current implementation doesn't introduce a new field, and introducing fields turned out to be very brittle