Hi Alex,
The update looks good.
Thanks,
Serguei
On 3/9/20 12:15, Alex Menkov wrote:
Updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/ThreadMXBean_Locks_test/webrev.02/
Changes are in LockFreeLogger comments only.
--alex
On 03/08/2020 21:19, David Holmes wrote:
P.S.
Forgot to note however that you need to update the documentation for
the logger now as the mention of "per-thread logs" makes no sense
now. Also in the spirit of not using @author, and because this is no
longer the code created by Jaroslav, please delete the @author line.
Thanks,
David
On 9/03/2020 2:15 pm, David Holmes wrote:
Hi Alex,
On 6/03/2020 4:54 am, Alex Menkov wrote:
Hi David,
Thanks you for the review.
On 03/04/2020 17:50, David Holmes wrote:
Hi Alex,
On 5/03/2020 10:30 am, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8240340
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/ThreadMXBean_Locks_test/webrev/
changes:
- assertThreadState method: don't re-read thread state throwing
exception (as we got weird error like "Thread WaitingThread is at
WAITING state but is expected to be in Thread.State = WAITING");
- added proper test shutdown on error (made all threads "daemon",
interrupt waiting thread if CheckerThread throws exception);
- if CheckerThread detects error, propagate the exception to main
thread;
The test changes seem fine.
- fixed LockFreeLogger class - it should work for logging from
several threads, but it doesn't. I prefer to simplify it just to
keep ConcurrentLinkedQueue<String>.
LockFreeLogger is also used by ThreadMXBeanStateTest test, but
only by a single thread.
I don't understand your changes here as you've completely changed
the intended design of the logger. The original accumulates log
entries per-thread and then spits them all out (though I'm not
clear on the exact ordering - I don't how to read that stream
stuff). The new code just creates a single queue of log records
interleaving entries from different threads. The simple logger may
be all that is needed but it seems quite different to the intent
of the original.
Testing changes in the test I discovered that there is something
wrong with the logger - it printed only part of the records, so I
have to look at the LockFreeLogger class and I don't understand how
it was supposed to work.
About ordering in cumulative log: each record has Integer which
used to sort log entries from all threads (i.e. records from
different threads are printed at the order which log() was called).
Looking at allRecords/records stuff I don't understand how it
should be used. To get logs from different threads in one logger,
we needs one instance. So we create LockFreeLogger (in main thread)
and ctor creates ThreadLocal record and register it in allRecords.
Logging from main thread works fine, but if any other thread tries
to log, 1st log() call creates its own ThreadLocal records (by
records.get()) and log records from this thread go there. But this
ThreadLocal records is not registered in allRecords, so this
logging won't be included in final log.
Looks like we need to change log() to something like
Map<Integer, String> recs = records.get();
if (recs.isEmpty()) {
allRecords.add(recs);
}
recs.put(id, String.format(format, params));
Yep good catch - this logger was completely broken.
But all this stuff do exactly the same as simple
ConcurrentLinkedQueue (i.e. lock free ordered list).
At least I don't see other rationale in the stuff.
I'm not certain of intent with the original but I'd always want to
see log entries in chronological order - which is what we now
clearly have.
Thanks,
David
--alex
Thanks,
David
--alex
