RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads

Leonid Mesnik leonid.mesnik at oracle.com
Tue Apr 30 20:02:09 UTC 2019


Test looks good.

Leonid

On 4/30/19 12:03 PM, coleen.phillimore at oracle.com wrote:
>
> I see.  I've rewritten the test as suggested:
> http://cr.openjdk.java.net/~coleenp/2019/8074355.06/webrev/test/hotspot/jtreg/runtime/Shutdown/ShutdownTest.java.html
>
> Whole (non-incremental) webrev is here.  It only has minor changes to 
> mutex.cpp and oopStorage.cpp.
>
> http://cr.openjdk.java.net/~coleenp/2019/8074355.06/webrev 
> <http://cr.openjdk.java.net/~coleenp/2019/8074355.06/webrev/test/hotspot/jtreg/runtime/Shutdown/ShutdownTest.java.html>
>
> Thanks!
> Coleen
>
> On 4/30/19 11:57 AM, Leonid Mesnik wrote:
>> Hi
>>
>> I mean that you could re-write your test like:
>>
>> /*
>>   * @test
>>   * @bug 4656449 4945125 8074355
>>   * @summary Make MutexLocker smarter about non-Java threads
>>   * @library /test/lib /runtime/testlibrary
>>   * @run main/driver ShutdownTest */
>>
>> public class ShutdownTest {
>>
>>     public static void main(String[] args) throws Throwable {
>>         List<String> options = new ArrayList<>();
>>
>> // Combine VM flags given from command-line and your additional options
>>         Collections.addAll(options, Utils.getTestJavaOpts());
>> Collections.addAll(options,
>>                 "-Xmx2500k",
>>                 "-XX:+UnlockDiagnosticVMOptions",
>>                 "-XX:+VerifyBeforeExit",
>>         );
>>         options.add(ShutdownTestThread.class.getName());
>>
>>         for (int iteration = 0; iteration < 30; ++iteration) {
>>             startVM(options);
>>         }
>>     }
>>
>>     private static void startVM(List<String> options) throws 
>> Throwable, RuntimeException {
>>         OutputAnalyzer out = 
>> ProcessTools.executeTestJvm(options.toArray(new String[options.size()]));
>> output.shouldContain("- ShutdownTest -");
>> output.shouldHaveExitValue(0);
>>
>>     }
>>
>>    static class ShutdownTestThread extends Thread {
>>     ...
>>     }
>>
>> }
>>
>> Also I just find that following 'obj' in lined 47 is not used. The 
>> allocation in line 50 might be optimized. Why is it needed?
>>    47    Object[] obj;
>>    48
>>    49    ShutdownTest() {
>>    50        obj = new Object[100000];
>>    51    }
>> Leonid
>>
>>> On Apr 30, 2019, at 8:27 AM, coleen.phillimore at oracle.com 
>>> <mailto:coleen.phillimore at oracle.com> wrote:
>>>
>>>
>>>
>>> On 4/29/19 11:14 PM, Leonid Mesnik wrote:
>>>> Hi
>>>>
>>>> A couple of comments about test.
>>>>
>>>> I see that test run process with selected GC and some other 
>>>> options. Original test just propagated all external options.
>>>>
>>>> Would not be easier to rewrite test like
>>>> http://hg.openjdk.java.net/jdk/jdk/file/tip/test/hotspot/jtreg/gc/g1/logging/TestG1LoggingFailure.java
>>>> so it combine all external options with '-XX:+VerifyBeforeExit'?
>>>
>>> I'm not sure what you mean.  This test above only tests G1.   I 
>>> think this test follows what the GC team wants to do for testing: if 
>>> an option is specified globally for the test, we pass that option 
>>> down through the process builder that way.   I did forget to fix it 
>>> now that we now have GC.selected() so the test can just do:
>>>
>>>     public static void main(String[] args) throws Exception {
>>>         for (int i = 0; i < 30; i++) {
>>>             test(GC.selected());
>>>         }
>>>     }
>>>
>>>>
>>>> In this case doesn't need WB anymore. It also could be a 'driver' 
>>>> and not 'othervm'.
>>>>
>>>> Also I've thought that "-XX:+UnlockDiagnosticVMOptions" should be 
>>>> used instead of  "-XX:+UnlockExperimentalVMOptions" for "-XX:+ 
>>>> VerifyBeforeExit" option.
>>>
>>> Good catch!  I need both UnlockDiagnosticVMOptions for 
>>> VerifyBeforeExit in product, and -XX:+UnlockExperimentalVMOptions 
>>> for UseZGC.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>> Leonid
>>>>
>>>>> On Apr 29, 2019, at 6:23 PM, coleen.phillimore at oracle.com 
>>>>> <mailto:coleen.phillimore at oracle.com> wrote:
>>>>>
>>>>>> I was expecting the constructor to check the name.
>>>>>
>>>>> Ok, this is a good idea.   I'm testing this through mach5 now but 
>>>>> it passes the safepoint tests.
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/2019/8074355.05/webrev
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 4/29/19 8:22 PM, David Holmes wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> I'll respond to a couple of things here relating to the new 
>>>>>> webrev and your comments below ...
>>>>>>
>>>>>> On 30/04/2019 8:21 am, coleen.phillimore at oracle.com wrote:
>>>>>>>
>>>>>>> An updated webrev is below.
>>>>>>>
>>>>>>>
>>>>>>> On 4/29/19 8:11 AM, coleen.phillimore at oracle.com wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/28/19 8:42 PM, David Holmes wrote:
>>>>>>>>> Hi Coleen,
>>>>>>>>>
>>>>>>>>> On 27/04/2019 2:10 am, coleen.phillimore at oracle.com wrote:
>>>>>>>>>> Summary: Use safepoint_check_always/safepoint_check_never 
>>>>>>>>>> instead of safepoint_check_sometimes for locks that are taken 
>>>>>>>>>> by JavaThreads and non-JavaThreads
>>>>>>>>>
>>>>>>>>> To clarify: the safepoint_check_[always|never|sometimes] 
>>>>>>>>> pertains only to the behaviour of JavaThreads that use the 
>>>>>>>>> lock, independently of whether a lock may be used by both 
>>>>>>>>> JavaThreads and non-JavaThreads.
>>>>>>>>
>>>>>>>> Yes.
>>>>>>>>>
>>>>>>>>>> This patch moves all but 3 of the locks to not be 
>>>>>>>>>> safepoint_check_sometimes. We have plans to fix these three.  
>>>>>>>>>> Also,
>>>>>>>>>
>>>>>>>>> So have you established that the reasons these were 
>>>>>>>>> 'sometimes' locks no longer apply and so it is correct to 
>>>>>>>>> change them? Or are you relying on testing to expose any 
>>>>>>>>> mistaken assumptions?
>>>>>>>>
>>>>>>>> Oh, I hope not.   Robbin and I have been looking at them and he 
>>>>>>>> thinks we can change them for the situations that they had to 
>>>>>>>> be sometimes locks.  The Heap_lock for example, couldn't be 
>>>>>>>> taken with a safepoint check on the exit path.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> this patch allows for being explicit about safepoint checking 
>>>>>>>>>> or not when the thread is a non-java thread, which is 
>>>>>>>>>> something that Kim objected to in my first patch.
>>>>>>>>>
>>>>>>>>> I don't understand what you mean by this. NJTs can currently 
>>>>>>>>> call either lock() or lock_without_safepoint_check().
>>>>>>>>>
>>>>>>>>
>>>>>>>> My first patch added the change for NJTs to just call lock and 
>>>>>>>> didn't call lock_without_safepoint_check for the 
>>>>>>>> safepoint_check_always flags.   Now they can call either.   My 
>>>>>>>> first patch also made Heap_lock an always lock, which it can't be.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> Tested with mach5 tier1-3.
>>>>>>>>>>
>>>>>>>>>> open webrev at 
>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8074355.03/webrev
>>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8074355
>>>>>>>>>
>>>>>>>>> src/hotspot/share/gc/shared/oopStorage.cpp
>>>>>>>>>
>>>>>>>>> How can these mutexes go from currently always needing 
>>>>>>>>> safepoint checks to now never needing them? Are they in fact 
>>>>>>>>> never used by JavaThreads?
>>>>>>>>>
>>>>>>>> Now, this asserts that they can't be sometimes either.  They 
>>>>>>>> asserted that they couldn't be "always" locks.  These locks are 
>>>>>>>> low level locks and should never safepoint check.
>>>>>>>>
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> src/hotspot/share/runtime/mutex.hpp
>>>>>>>>>
>>>>>>>>>  95   void check_safepoint_state (Thread* self, bool 
>>>>>>>>> safepoint_check) NOT_DEBUG_RETURN;
>>>>>>>>>
>>>>>>>>> Please use the same parameter name as in the implementation: 
>>>>>>>>> do_safepoint_check.
>>>>>>>>
>>>>>>>> Fixed.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 109   // Java and NonJavaThreads. When the lock is initialized 
>>>>>>>>> with _safepoint_check_always,
>>>>>>>>>  110   // that means that whenever the lock is acquired by a 
>>>>>>>>> JavaThread, it will verify that each
>>>>>>>>>  111   // acquition from a JavaThread is done with a safepoint 
>>>>>>>>> check.
>>>>>>>>>
>>>>>>>>> That can simplify to just:
>>>>>>>>>
>>>>>>>>> 109   // Java and NonJavaThreads. When the lock is initialized 
>>>>>>>>> with _safepoint_check_always,
>>>>>>>>> 110   // that means that whenever the lock is acquired by a 
>>>>>>>>> JavaThread, it will verify that
>>>>>>>>> 111   // it is done with a safepoint check.
>>>>>>>>
>>>>>>>> That's better and less redundant.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Should we then continue:
>>>>>>>>>
>>>>>>>>> 111   // it is done with a safepoint check. In corollary when 
>>>>>>>>> the lock
>>>>>>>>> 112   // is initialized with _safepoint_check_never, that 
>>>>>>>>> means that
>>>>>>>>> 113   // whenever the lock is acquired by a JavaThread it will 
>>>>>>>>> verify
>>>>>>>>> 114   // that it is done without a safepoint check.
>>>>>>>>>
>>>>>>>>> ?
>>>>>>>>
>>>>>>>> I like it.  Added with some reformatting so the paragraph is 
>>>>>>>> same width.
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 38   SafepointCheckRequired not_allowed = do_safepoint_check ? 
>>>>>>>>> _safepoint_check_never : _safepoint_check_always;
>>>>>>>>> 39 assert(!self->is_Java_thread() || _safepoint_check_required 
>>>>>>>>> != not_allowed,
>>>>>>>>>
>>>>>>>>> I found this code very difficult to understand due to some 
>>>>>>>>> previous choices. All of the names that start with underscore 
>>>>>>>>> give the illusion (to me) of being variables (or at least 
>>>>>>>>> being the same kind of thing) but two are enum values and one 
>>>>>>>>> is a field. Using this->_safepoint_check_required would at 
>>>>>>>>> least make it clearer which is the field.
>>>>>>>>
>>>>>>>> Ew. no. The underscore makes it clear it's a field of the class 
>>>>>>>> Monitor.
>>>>>>>
>>>>>>> I don't know the convention for enum values, but maybe these 
>>>>>>> could say Monitor::_safepoint_check_never etc instead.  That's a 
>>>>>>> typical convention we use even inside member functions of the class.
>>>>>>
>>>>>> Yes - thanks! That looks much clearer (though I admit enums 
>>>>>> confuse me in C++, in particular with named enums I would have 
>>>>>> expected to see SafepointCheckRequired::_safepoint_check_never 
>>>>>> [which would be quite unwieldy] rather than 
>>>>>> Monitor::_safepoint_check_never.)
>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>  43   // Allow NonJavaThreads to lock and wait with a 
>>>>>>>>> safepoint check for locks that may be shared with JavaThreads.
>>>>>>>>>  44 assert(self->is_Java_thread() || !do_safepoint_check || 
>>>>>>>>> _safepoint_check_required != _safepoint_check_never,
>>>>>>>>>  45          "NonJavaThreads can only check for safepoints if 
>>>>>>>>> shared with JavaThreads");
>>>>>>>>>
>>>>>>>>> This is very confusing: NJTs don't do safepoint checks. I 
>>>>>>>>> think what you mean here is that you will allow a NJT to call 
>>>>>>>>> lock() rather than lock_without_safepoint_check() but only if 
>>>>>>>>> the mutex is "shared with JavaThreads". But 
>>>>>>>>> always/sometimes/never has nothing to with whether the lock is 
>>>>>>>>> shared between JTs and NJTs. I understand that a NJT-only 
>>>>>>>>> mutex should, by convention, be created with 
>>>>>>>>> _safepoint_check_never - but it really makes no practical 
>>>>>>>>> difference. Further, a mutex used only by JavaThreads could in 
>>>>>>>>> theory also be _safepoint_check_never.
>>>>>>>>
>>>>>>>> It is confusing but this found some wild use of a lock(do 
>>>>>>>> safepoint check) call for a lock that is now defined as 
>>>>>>>> safepoint_check_never. The shared lock commentary was because 
>>>>>>>> for a shared lock, it can be acquired with the safepoint_check 
>>>>>>>> parameter from a NonJava thread.
>>>>>>>>
>>>>>>>> Maybe it should say this instead:
>>>>>>>>
>>>>>>>>   // NonJavaThreads defined with safepoint_check_never should 
>>>>>>>> never ask to safepoint check.
>>>>>>>> assert(thread->is_Java_thread() || !do_safepoint_check || 
>>>>>>>> _safepoint_check_required != _safepoint_check_never,
>>>>>>>>          "NonJavaThread should not check for safepoint");
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 47   // Only Threads_lock, Heap_lock and SR_lock may be 
>>>>>>>>> safepoint_check_sometimes.
>>>>>>>>> 48 assert(_safepoint_check_required != 
>>>>>>>>> _safepoint_check_sometimes || this == Threads_lock || this == 
>>>>>>>>> Heap_lock ||
>>>>>>>>> 49          this->rank() == suspend_resume,
>>>>>>>>> 50          "Lock has _safepoint_check_sometimes %s", 
>>>>>>>>> this->name());
>>>>>>>>>
>>>>>>>>> This assert belongs in the constructor, not in every lock 
>>>>>>>>> operation, as it depends only on the monitor instance not on 
>>>>>>>>> the thread doing the lock.
>>>>>>>>>
>>>>>>>>
>>>>>>>> You're right, that's much better!  Fixed.
>>>>>>>
>>>>>>> This isn't better because I can't check this == Threads_lock 
>>>>>>> when calling the constructor on Threads_lock.  This code will go 
>>>>>>> away when the "sometimes" locks are fixed though.
>>>>>>
>>>>>> I was expecting the constructor to check the name.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8074355.04/webrev
>>>>>>>
>>>>>>> Sorry for not doing incremental. The only changes were in 
>>>>>>> mutex.cpp/hpp.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Coleen
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Coleen
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>


More information about the hotspot-dev mailing list