On Wed, 11 Nov 2020 22:55:16 GMT, Daniel D. Daugherty <dcu...@openjdk.org> wrote:
>> As the stack trace in the bug shows, we cannot load classes, since we may >> take a monitor. >> Resulting in an unexpected result to GetCurrentContendedMonitor(). >> Trying to use some decent primitive, e.g. Wicket/Semaphore/.., without being >> implementation dependent means we must warm up every possible scenario, >> since it may use a new class. >> Instead I here just use sleep + volatile for the barriers. >> >> I cannot reproduce with these changes. >> >> Chewing through T6 as most issues have been seen there. > > I like the fix. Taking Wicket out of play will help with class loading > issues in this test. You don't really need to deal with unexpected > locking events from the ObjectLocker usage by class loading. > Hi Robbin, > > It isn't at all clear to me exactly what class initialization is happening, > when, and how the contention is arising. I see in the bug report a stack dump > for wicket.unlock and cannot see how unlock can trigger class initialization > when all classes would have been needed by the lock. Hey, David. Different paths uses different classes. In this case it's the AbstractQueuedSynchronizer that uses LockSupport if a thread have already been put to sleep. If we call unlock() before the other thread hit the parker in waitFor(), LockSupport is not used, and thus not loaded. Without any other synchronization we do not know when it will loaded LockSupport. Having a second synchronization we do not need the Wicket, hence may change removes the Wicket. As I said, we need to exercise all code paths (since any difference in timing may result in a different path), to do that reliable, we must use a second primitive. But it very to hard to guarantee that we actually manage that. Implementing some kind of warm-up in a static initializer in the Wicket might be doable. > That aside avoiding the class loading/initialization seems more of a > workaround. Ideally the test would be more discriminating about what monitors > it checks for; or more accurately ensures that the code has reached the right > place before doing the check. Avoiding loading class and "accurately ensures that the code has reached the right place" is the same thing here, since unlock may do class loading thus making sure we do not call unlock() at the same time as we do GetCurrentContendedMonitor solves this. I looked at other primitives (which are not using synchronized), I saw none that would be any better than the Wicket (class loading-wise). In most use-cases of the Wicket there is no apparent reason why a j.u.c.Semaphore could not be used instead, maybe all. So I'm not even sure the Wicket is still something worth pursuing. Thanks, Robbin > > Thanks, > David ------------- PR: https://git.openjdk.java.net/jdk/pull/1177
