RFR: 8241123: Refactor vmTestbase stress framework to use j.u.c and make creation of threads more flexible
Leonid Mesnik
leonid.mesnik at oracle.com
Fri Mar 20 01:10:28 UTC 2020
Hi
Thank you for review and feedback. See my comments inline.
> On Mar 19, 2020, at 6:03 PM, serguei.spitsyn at oracle.com wrote:
>
> Hi Leonid,
>
> It looks good in general.
> Just a couple of comments.
>
>
> http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/Wicket.java.frames.html <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/Wicket.java.frames.html>
> 168 public int waitFor(long timeout) {
> 169 if (timeout < 0)
> 170 throw new IllegalArgumentException(
> 171 "timeout value is negative: " + timeout);
> 172
> 173 long id = System.currentTimeMillis();
> 174
> 175 try {
> 176 lock.lock();
> 177 --waiters;
> 178 if (debugOutput != null) {
> 179 debugOutput.printf("Wicket %d %s: waitFor(). There are %d waiters totally now.\n", id, name, waiters);
> 180 }
> 181
> 182 long waitTime = timeout;
> 183 long startTime = System.currentTimeMillis();
> 184
> 185 while (count > 0 && waitTime > 0) {
> 186 try {
> 187 condition.await(waitTime, TimeUnit.MILLISECONDS);
> 188 } catch (InterruptedException e) {
> 189 }
> 190 waitTime = timeout - (System.currentTimeMillis() - startTime);
> 191 }
> 192 --waiters;
> 193 return count;
> 194 } finally {
> 195 lock.unlock();
> 196 }
> 197 }
>
> The waiters probably needs to be incremented instead of decremented at line:
> 177 --waiters;
Thank you, fixed.
>
> http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/runner/ThreadsRunner.java.udiff.html <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/runner/ThreadsRunner.java.udiff.html>
> private void waitForOtherThreads() {
> if (shouldWait) {
> shouldWait = false;
> - finished.unlock();
> - finished.waitFor();
> + finished.decrementAndGet();
> + while (finished.get() != 0) {
> + try {
> + Thread.sleep(1000);
> + } catch (InterruptedException ie) {
> + }
> + }
> } else {
> throw new TestBug("Waiting a second time is not premitted");
> }
> }
>
> Should we use a shorter sleep, something like Thread.sleep(100)?
>
These tests executed 30 or 60 seconds now by default, so sleeping 1 sec doesn't increase overall time. But tI am fine to change it 100, it also should works fine.
Leonid
>
> Thanks,
> Serguei
>
>
> On 3/18/20 15:18, Leonid Mesnik wrote:
>>
>> On 3/18/20 2:30 PM, Igor Ignatyev wrote:
>>>> I need more time to get grasp of Wicket and your changes in it; will come back to you after I understand them.
>>> ok, now when I believe that I have enough understanding of Wicket, I have a few comments:
>>> 1.
>>>> 68 private Lock lock = new ReentrantLock();
>>>> 69 private Condition condition = lock.newCondition();
>>> it's better to make these fields final.
>>>
>>> 2. as all writes and reads of Wicket::count are guarded by lock.lock, there is no need for it to be atomic.
>>> 3. adding lock to getWaiters will also remove need for Wicket::waiters to be atomic.
>> All 3 are fixed. Thanks for your suggestions.
>>
>> Updated version:
>>
>> http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/ <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/>
>> Leonid
>>
>>>
>>> the rest looks good to me.
>>>
>>> Thanks,
>>> -- Igor
>>>
>>>
>>>
>>>> On Mar 18, 2020, at 12:48 PM, Igor Ignatyev <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
>>>>
>>>> Hi Leonid,
>>>>
>>>> I've started looking at your webrev, and so far have a couple questions:
>>>>
>>>>> Test vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects003/referringObjects003a.java was updated to don't use Wicket. (The lock has a reference to thread which affects test.)
>>>> can't you use just a volatile boolean field?
>>>>
>>>>> Wicket "finished" in class ThreadsRunner was changed to atomicInt/sleep to avoid OOME in j.u.c.l.Condition::await() which might happened in stress GC tests.
>>>> won't j.u.c.CountDownLatch be more appropriate and cleaner solution here?
>>>>
>>>> I need more time to get grasp of Wicket and your changes in it; will come back to you after I understand them.
>>>>
>>>> -- Igor
>>>>
>>>>> On Mar 18, 2020, at 12:37 PM, Leonid Mesnik <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> Could you please review following fix which slightly refactor vmTestbase stress test harness. This refactoring helps to add virtual threads testing support.
>>>>>
>>>>> The Wicket uses plain sync/wait/notify mechanism which cause carrier thread starvation and should not be used in virtual threads. The ManagedThread is a subclass of Thread so it couldn't be virtual thread.
>>>>>
>>>>>
>>>>> Following fix changes Wicket to use locks/conditions to don't pin vthread to carrier thread while starting testing.
>>>>>
>>>>> ManagedThread is fixed to keep execution thread as the thread variable and isolate it's creation.
>>>>>
>>>>> Test vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects003/referringObjects003a.java was updated to don't use Wicket. (The lock has a reference to thread which affects test.)
>>>>>
>>>>> Wicket "finished" in class ThreadsRunner was changed to atomicInt/sleep to avoid OOME in j.u.c.l.Condition::await() which might happened in stress GC tests.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8241123/webrev.00/ <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.00/>
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8241123 <https://bugs.openjdk.java.net/browse/JDK-8241123>
>>>>>
>>>>>
>>>>> Leonid
>>>>>
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200319/93c61621/attachment-0001.htm>
More information about the serviceability-dev
mailing list