RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Apr 30 20:08:00 UTC 2019
Thanks, Leonid - thank you for your help!
Coleen
On 4/30/19 4:02 PM, Leonid Mesnik wrote:
>
> 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