Operating System: All
Platform: All

Proposed patch:

https://git.opendaylight.org/gerrit/#/c/7892/

It looks like the result of the code here is still incorrect. This is attempting to do a double-checked lock pattern in an incorrect way with the Java memory model. See http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
Here's the incorrect code:

public static List<Range<BigInteger>> length() {
if (_length == null) {
synchronized (org.opendaylight.yang.gen.v1.urn.opendaylight.groupbasedpolicy.common.rev140421.Name.class) {
if (_length == null)

}
}
return _length;
}

In this case the (easy) correct fix is to use the static singleton pattern by declaring the _length field as a static variable in a subclass with a static initializer. It will be automatically initialized on the first access just as the code here is attempting to do.

Agreed, that double-checked locking is my mistake (and now I have to hunt for all the paces I proliferated it). Not sure what an inner class will cost us in terms of memory overhead. We may need to do some profiling and tradeoff juggling.

Given that this is a static variable the additional memory overhead is irrelevant. If you're doing this on an instance variable just setting it as volatile should fix your code.

So we have the following options:

1) do proper locking by protecting the entire check in a synchronized block
2) make _length volatile
3) inner holder class
4) allocate eagerly at class load time

I would opt for 3), except it may enlarge our PermGen footprint. I suspect we are usually very close to allocating an object when we touch the class, so 4) may actually make sense, too.

Opinions?

Well, the holder is a class, which needs to be loaded into pergen and be left there. While it is insignificant overhead compared to objects, given that we have thousands of classes, that overhead may pile up.

Oh I thought you'd done the lazy initialization because you were benchmarking this. If not, I'd say just move the initialization to a static block pending some evidence that lazy loading is useful.

Thinking about this a bit more, we do not need any locking at all. The reasoning is the following:

• we are materializing immutable state, which is based solely on class bytecode
• the encapsulating object is not referenced by individual instances

There are no ill side-effects from threads seeing different instances of the state – since it is immutable, it is bound to behave the same. Since they are not referenced by instances, we are not running the risk of leaking multiple instances to the heap in general. Resolution of which instance will actually survive is amortized.

Does this sounds correct, or am I missing something?

+1 on removing double-check for null (it creates Findbug issue).

So let's lose the lazy initialization and perform it on class load, using normal constant field. That should cut the complexity.

proposed patch: https://git.opendaylight.org/gerrit/#/c/9483/