On 25.07.2017 15:03, Jim Graham wrote:
On 7/25/17 12:53 PM, Sergey Bylokhov wrote:
In this variant the field should be volatile, and this will introduce
some synchronization. Otherwise it will be possible to read non-null
value in localEnv while the constructor of GraphicsEnvironment was not
completed on some other thread(but the field was assigned already > > Even though the assignment is the last statement in the method?
The assignment include these operations:
- Allocate memory
- Initialize the object
- Assign the object to the field.
These operation may be reordered, so some other thread can see non-null
reference in the field while initialization was not complete.
In looking through the article that you sent out (only briefly) it looks
like the consideration is that the initialization method run in one
thread could reorder the instructions at run time so that the field is
assigned before the initialization steps are done, even if the code is
written with a different specific ordering.
I'm guessing that the Java Memory Model ensures that membars are placed
to protect static field initialization from similar dangers?
The key feature here is initialization of the class.
The next line will be blocked while the LocalGE class will not be
initialized:
return LocalGE.INSTANCE;
And initialization will start only when this code will run for the first
time.
If that is the case then I think we have a few places where we do this
"manual conditional synchronization" that should probably be
investigated. If I remember correctly, though, they may use the
following paradigm which might not suffer from that issue:
Does it help if a local variable is used, as in:
GE env = this.localEnv
if (env == null) {
synchronized (class) {
if (this.localEnv == null) {
localEnv = createGE();
}
env = this.localEnv;
}
}
return env;
This ensures that the second read from the field is done in the
synchronized block for the case that it was originally found to be null,
though this might not help if the line "localEnv = createGE()" can
reorder code from inside the call to createGE() to code outside of it.
However those dangling reordered stores should be resolved before it
leaves the synchronized block, no?
I think that it is possible that one thread will do:
- create an object
- assign the object to the localEnv.
- Initialize the object.
And the second thread will do:
- Read non-null value from the this.localEnv to the env
- Check env to null
- Return non-initialized object.
In any case, I can see that the code used in the webrev will eventually
have no dead code at all in the final compiled version of the method
whereas this version would minimally at least have a null check that
must be executed even though it would be vestigial once the
initialization was done...
+1 for the inner class version - it's the most straightforward way to do
this optimally...
--
Best regards, Sergey.
