Re: RFR: 8241123: Refactor vmTestbase stress framework to use j.u.c and make creation of threads more flexible

Thu, 19 Mar 2020 19:06:24 -0700

Than you for fixing.
Then I do not need to see another webrev.

Thanks,
Serguei


On 3/19/20 18:10, Leonid Mesnik wrote:
Hi

Thank you for review and feedback. See my comments inline.

On Mar 19, 2020, at 6:03 PM, [email protected] 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
 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
         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/

Leonid


the rest looks good to me.

Thanks,
-- Igor



On Mar 18, 2020, at 12:48 PM, Igor Ignatyev <[email protected]> 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 <[email protected]> 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/

bug: https://bugs.openjdk.java.net/browse/JDK-8241123


Leonid






Reply via email to