RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Apr 30 19:03:09 UTC 2019
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