Hi Claes,
On 06/18/2018 12:02 PM, Claes Redestad wrote:
On 2018-06-18 05:45, David Holmes wrote:
Making it "final" to fix unsafe publication only works with actual
publication after construction. You're making it "final" but then not
setting it at construction time and instead using UNSAFE.putVolatile
to get around the fact that it is final! The "final" serves no
purpose in that regard. Further a putVolatile without corresponding
getVolatile does not achieve safe publication under the JMM.
Adding a volatile read on every read through Properties is likely to
have some performance impact,
On non-Intel architectures in particular. On intel it would just inhibit
some JIT optimizations like hoisting the read of field out of loop...
but it could still be better than before 8029891 where such operations
were synchronized. Regarding Peter's comments about defaults, I think
accesses to that field was implicitly guarded by the synchronized
super.get calls before 8029891 and now needs to be re-examined too.
This was pre- 8029891 code of getProperty for example:
public String getProperty(String key) {
Object oval = super.get(key);
String sval = (oval instanceof String) ? (String)oval : null;
return ((sval == null) && (defaults != null)) ?
defaults.getProperty(key) : sval;
}
So 'defaults' field was accessed out of synchronized super.get() method
and getProperty was not synchronized. This is pre-existing issue. But
'map' field is new.
The problem with 'defaults' field is also that is is protected and
subclasses can access it without synchronization. If it is made
volatile, getProperty() method will be penalized.
So maybe using weaker memory fences in combination with plain 'property'
field can make the field behave like it was final, so at least accesses
to field within Properties class can be made non-racy. If this is done
for 'properties' then same fences will work for 'map' too and it doesn't
have to be final.
I think making the actual field(s) volatile is the only JMM compliant
way to correctly address this.
The only issue is serialization-related readHashTable method (clone
could be implemented without Unsafe as clone.map.putAll(map)).
1st you would have to do:
clone.map = new CHM();
and only then:
clone.map.putAll(map);
or else you would be adding the elements of the original map to the
original map (which would actually work with CHM), but then the cloned
Properties object would share the map with original one. So you do need
to assign to 'map' field in clone().
Is there no sanctioned way (using e.g. VarHandles) to safely construct
and publish a transient "final" field in the scope of a readObject?
I've been shying away from experimenting with VarHandles in places
like Properties as I'm certain it could lead to bootstrap issues, but
for something like a deserialization hook it might very well be fine.
Paul would know for sure, but I think this was deliberately prevented in
VarHandle(s).
Regards, Peter
/Claes
