RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads
Leonid Mesnik
leonid.mesnik at oracle.com
Tue Apr 30 15:57:04 UTC 2019
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 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 <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 <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 <mailto:coleen.phillimore at oracle.com> wrote:
>>>>>
>>>>> An updated webrev is below.
>>>>>
>>>>>
>>>>> On 4/29/19 8:11 AM, coleen.phillimore at oracle.com <mailto: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 <mailto: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 <http://cr.openjdk.java.net/~coleenp/2019/8074355.03/webrev>
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8074355 <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 <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