On 8/30/13 3:27 AM, David Holmes wrote:
Hi Daniel,
As usual with these kinds of changes we start by trying to
complete an
incomplete/incorrect synchronization pattern and get diverted into
changing the synchronization pattern. :) The latter of course has
more
risk than the former.
Making everything synchronized has the benefit that we don't need to
analyse the "real" methods to check what would happen if one of these
data values changed mid-method. Might be harmless, might not.
Atomicity makes things easy to reason about.
Agreed. We can analyze the 'real methods' of Handler classes in the
JDK.
But I'm sure there are plenty of
subclasses of handlers in applications out there that we don't know
about. So we can't really be sure
what the effect of changing synchronization might have on those
subclasses.
Making the fields volatile and un-sync'ing the getters is a potential
optimization for readers. But are these fields that are read
frequently by multiple threads?
At least three of them are accessed by isLoggable() which I believe is
called very frequently by
multiple threads (level, filter, and writer).
The downside of doing so is a negative performance impact inside the
"real" methods where every volatile field access requires
(platform-dependent) memory barriers.
That - I think - should not be such an issue since the fields are
private.
I mean - we can see whether there are methods that do multiple
access to
the fields in the
classes that define these fields - and fix that if necessary.
For subclasses, they will have to call the getter, which will require
synchronization anyway.
I mean - for subclasses - isn't the fact that the getter accesses a
volatile field or the fact that the
getter is synchronized equivalent in terms of performance?
JDK 8 is a major version of the JDK - so I think if we want to fix
this
bug it's
probably better to do it now rather than in a minor update.
It seems we have 5 choices:
a. Not fixing
b. Using regular 'synchronized' pattern [1]
c. Using volatiles, synchronize setters only [2]
d. Using volatiles, only synchronize setters when obviously necessary
e. other? (like introducing new private locks - but I don't think
anybody would want that)
[1] <http://cr.openjdk.java.net/~dfuchs/webrev_6823527/webrev.00/>
[2] <http://cr.openjdk.java.net/~dfuchs/webrev_6823527/webrev.01/>
My own preference would lean to either b) or c).
-- daniel
David
On 30/08/2013 5:54 AM, Daniel Fuchs wrote:
On 8/29/13 9:45 PM, Mandy Chung wrote:
On 8/29/13 11:04 AM, Daniel Fuchs wrote:
On 8/29/13 7:58 PM, Mandy Chung wrote:
On 8/29/13 9:46 AM, Daniel Fuchs wrote:
Here is the new webrev implementing Jason's suggestion.
Compared to previous webrev only Handler.java &
StreamHandler.java
have changed.
<http://cr.openjdk.java.net/~dfuchs/webrev_6823527/webrev.01/>
Looks good. I also agree that making those fields to be volatile
is a
good suggestion and only requires writer to acquire the lock.
A minor comment: The setter methods in Handler class are
synchronized
at the method entry whereas StreamHandler.setPushLevel
synchronizes
after checkPermission() is called. checkPermission doesn't need
to be
synchronized. Either way is fine with me and it'd be good to
do it
consistently.
Hi Mandy,
Are you suggesting I should put checkPermission() within
the synchronized block? It might be more consistent with
what the other setters do. Although as you state it does
not matter much.
Having a second thought - do the setters really need to be
synchronized? My guess is that StreamHandler.publish is
synchronized
so as to ensure that the log messages are sequential and not
interpersed
when multiple threads are publishing. The spec is unclear.
Perhaps it
worths looking into this further?
I believe at least setEncodings() needs synchronization.
setLevel() had it - so are we going to cause subtle race conditions
if we remove it?
My own feeling is that keeping setters synchronized might be better.
We could of course say that a subclass which needs to prevent
e.g. setFormatter() from being called concurrently with publish()
should override setFormatter just for the purpose of synchronizing
it...
One more minor note: MemoryHandler.pushLevel can also be made
volatile.
L262: look like the log manager is not needed in the existing code.
hmmm. Right. It's already called once in the super class so removing
this call should have no effect. I will remove it.
-- daniel
Mandy
