RFR: 8210832: Remove sneaky locking in class Monitor

Patricio Chilano patricio.chilano.mateo at oracle.com
Tue Feb 5 15:02:23 UTC 2019


On 2/5/19 8:59 AM, coleen.phillimore at oracle.com wrote:
> This looks great!
Thanks Coleen!


Patricio
> Coleen
>
> On 2/5/19 2:58 AM, Robbin Ehn wrote:
>> On 2019-02-05 01:06, David Holmes wrote:
>>> v6 incremental looks good to me.
>>
>> +1
>>
>> /Robbin
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>> On 5/02/2019 6:57 am, Patricio Chilano wrote:
>>>> Hi Dan,
>>>>
>>>> Here is v06. It also contains David's suggested changes for 
>>>> assert_owner(), and based on your correction about declaring 
>>>> pointers I fixed one in ThreadBlockInVMWithDeadlockCheck 
>>>> declaration. Files mutex.cpp and mutex.hpp have different styles of 
>>>> "pointer to" declarations so except the one you mentioned I didn't 
>>>> try to fix them all since I don't know if changing all of them to " 
>>>> * " is the right approach.
>>>>
>>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v06/inc/webrev
>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v06/webrev/
>>>>
>>>> Thanks,
>>>> Patricio
>>>>
>>>> On 2/4/19 2:47 PM, Daniel D. Daugherty wrote:
>>>>> On 2/4/19 2:40 PM, Patricio Chilano wrote:
>>>>>> Hi Dan,
>>>>>>
>>>>>> On 2/4/19 2:05 PM, Daniel D. Daugherty wrote:
>>>>>>> On 2/3/19 8:46 PM, Patricio Chilano wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> Here is v05:
>>>>>>>>
>>>>>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v05/webrev/
>>>>>>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v05/inc/
>>>>>>>
>>>>>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>>>>>>     L295: // - When transitioning in (constructor), it checks 
>>>>>>> for safepoints without blocking, i.e calls
>>>>>>>         nit - s/i.e /i.e., /
>>>>>>>         (You missed the change from my v04 review)
>>>>>> Done! I did made the change s/having to block/blocking from your 
>>>>>> review but didn't see the extra comma you added after i.e.
>>>>>
>>>>> Extra period and extra comma... I'm a font/typography nut... :-)
>>>>>
>>>>>
>>>>>>
>>>>>>> L299: class ThreadBlockinVMWithDeadlockCheck : public 
>>>>>>> ThreadStateTransition {
>>>>>>>     L312: ThreadBlockinVMWithDeadlockCheck(JavaThread *thread, 
>>>>>>> Monitor** in_flight_monitor_adr)
>>>>>>>     L326:   ~ThreadBlockinVMWithDeadlockCheck() {
>>>>>>> s/ThreadBlockinVMWithDeadlockCheck/ThreadBlockInVMWithDeadlockCheck/ 
>>>>>>>
>>>>>>>         Lower case 'i' -> 'I' to match the 
>>>>>>> CamelCastStyleInOtherHelpers.
>>>>>> Done.
>>>>>>
>>>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>>>     L36: static inline void assert_owner(Thread *owner, Thread 
>>>>>>> *thread) {
>>>>>>>         nit - the predominant style in this file is ' * ' (space 
>>>>>>> on each side of '*')
>>>>>> Ok, is that the style that we are supposed to follow in general ? 
>>>>>> Otherwise I can change all the ones that do not match the 
>>>>>> preferred style.
>>>>>
>>>>> That's a good question. The general rule is to follow the
>>>>> predominant style in the file. However, I'm not sure what
>>>>> the HotSpot recommended style is (I used to know this...).
>>>>>
>>>>>
>>>>>>
>>>>>>> L38:          "invalid owner: owner=" INTPTR_FORMAT ", should 
>>>>>>> be=" INTPTR_FORMAT,
>>>>>>>         Another possible style of mesg:
>>>>>>>                   "invalid owner: actual=" INTPTR_FORMAT ", 
>>>>>>> expect=" INTPTR_FORMAT,
>>>>>> I like more the first style, but if you really prefer the second 
>>>>>> one I can change it.
>>>>>
>>>>> I'm okay with what you have.
>>>>>
>>>>>
>>>>>>
>>>>>>> ThreadBlockinVMWithDeadlockCheck renames to 
>>>>>>> ThreadBlockInVMWithDeadlockCheck in
>>>>>>>     this file also.
>>>>>> Done.
>>>>>>
>>>>>>> src/hotspot/share/runtime/mutex.hpp
>>>>>>>     No comments.
>>>>>>>
>>>>>>> src/hotspot/share/runtime/safepointMechanism.inline.hpp
>>>>>>>     L83:   if (!uses_thread_local_poll() || 
>>>>>>> local_poll_armed(thread)) {
>>>>>>>         Did you decide this line does not need a comment at all?
>>>>>> How about adding the following comment based on your previous email:
>>>>>>
>>>>>> // If using thread local polls, we should not check the
>>>>>> // global_poll() and callback via block() if the VMThread
>>>>>> // has not yet armed the local poll. Otherwise, when used in
>>>>>> // combination with should_block(), the latter could miss
>>>>>> // detecting the same safepoint that this method would detect
>>>>>> // if only checking global polls.
>>>>>
>>>>> I'm good with that comment.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Patricio
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Tested tiers 1-6.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Patricio
>>>>>>>>
>>>>>>>> On 2/1/19 7:58 PM, Daniel D. Daugherty wrote:
>>>>>>>>> On 2/1/19 7:49 PM, David Holmes wrote:
>>>>>>>>>> <trimming>
>>>>>>>>>>
>>>>>>>>>> On 2/02/2019 7:48 am, Patricio Chilano wrote:
>>>>>>>>>>> On 2/1/19 4:28 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>>>>>>> In regards to the name ThreadLockBlockInVM ... I don't 
>>>>>>>>>>>>> have a good suggestion. 
>>>>>>>>>>>>> ThreadBlockInVMWithSafepointCheckingButOnlyBlockOnTheWayOut 
>>>>>>>>>>>>> is rather unwieldy. ;-) But the "Lock" part really doesn't 
>>>>>>>>>>>>> mean anything. So your suggestion of 
>>>>>>>>>>>>> ThreadBlockinVMWithDeadlockPrevention seems a big 
>>>>>>>>>>>>> improvement to me. :) 
>>>>>>>>>>>>
>>>>>>>>>>>> How about ThreadBlockInVMForLock ?   This answers the 
>>>>>>>>>>>> question "why" this class, vs. "what" this class does. 
>>>>>>>>>>>> Since the latter can change. 
>>>>>>>>>>> I like that name. Is that name okay with you David?
>>>>>>>>>>
>>>>>>>>>> Sorry no. "Lock" still adds zero information about when/why 
>>>>>>>>>> you would use this. It's used within Monitor::lock and 
>>>>>>>>>> Monitor::wait so is not specific to "locking" even in that 
>>>>>>>>>> class. This is about blocking in the VM and we need to know 
>>>>>>>>>> what this version does differently to the existing TBIVM. 
>>>>>>>>>> There are lots of different types of "locking" in the VM and 
>>>>>>>>>> we don't use this with them.
>>>>>>>>>>
>>>>>>>>>> Dan can be a tie-breaker on this bikeshed. Grab your brush 
>>>>>>>>>> Dan! ;-)
>>>>>>>>>
>>>>>>>>> ThreadBlockInVMBecauseSneakyLockingIsEvil?
>>>>>>>>>
>>>>>>>>> ThreadBlockinVMWithDeadlockPrevention is okay.
>>>>>>>>>
>>>>>>>>> ThreadBlockinVMWithDeadlockCheck
>>>>>>>>> ThreadBlockinVMWithDeadlockChk
>>>>>>>>> ThreadBlockinVMWithDeadlockCk (David won't like this one :-) )
>>>>>>>>>
>>>>>>>>> Just pick a name and run with it... :-)
>>>>>>>>>
>>>>>>>>> Dan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Patricio
>>>>>>>>>>>> thanks,
>>>>>>>>>>>> Coleen
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> David
>>>>>>>>>>>>> -----
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 1/02/2019 3:05 pm, Patricio Chilano wrote:
>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 1/31/19 12:54 AM, David Holmes wrote:
>>>>>>>>>>>>>>> On 31/01/2019 7:37 am, Patricio Chilano wrote:
>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 1/30/19 2:29 AM, David Holmes wrote:
>>>>>>>>>>>>>>>>> Hi Patricio,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> <trimming>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> First, thanks for all the many weeks of work you've 
>>>>>>>>>>>>>>>>> put into this, pulling together a number of ideas from 
>>>>>>>>>>>>>>>>> different people to make it all work!
>>>>>>>>>>>>>>>> Thanks! Credit to you for the PlatformMonitor 
>>>>>>>>>>>>>>>> implementation  : )
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> :) Nothing innovative there.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I've only got a few minor comments/suggestions.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 30/01/2019 10:24 am, Patricio Chilano wrote:
>>>>>>>>>>>>>>>>>> Full: 
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/webrev/ 
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Inc: 
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/inc/webrev/ 
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I'm very unclear how ThreadLockBlockInVM differs from 
>>>>>>>>>>>>>>>>> ThreadBlockInVM. You've duplicated a lot of complex 
>>>>>>>>>>>>>>>>> code which is masking the actual difference between 
>>>>>>>>>>>>>>>>> the two wrappers to me. It seems to me that an extra 
>>>>>>>>>>>>>>>>> arg to transition_and_fence should allow you to handle 
>>>>>>>>>>>>>>>>> the new behaviour without having to duplicate so much 
>>>>>>>>>>>>>>>>> of this code. In any case the semantics of 
>>>>>>>>>>>>>>>>> ThreadLockBlockInVM needs to be described.
>>>>>>>>>>>>>>>> I could do it with one extra argument, but I would need 
>>>>>>>>>>>>>>>> to add two extra branches in transition_and_fence(), 
>>>>>>>>>>>>>>>> one to decide if I'm in the Monitor case to avoid 
>>>>>>>>>>>>>>>> calling SafepointMechanism::block_if_requested() 
>>>>>>>>>>>>>>>> directly and another one to actually decide if I'm 
>>>>>>>>>>>>>>>> transitioning in or out, since the actions to perform 
>>>>>>>>>>>>>>>> are different. I think it is easier to read without 
>>>>>>>>>>>>>>>> adding new conditionals, and also we will save those 
>>>>>>>>>>>>>>>> extra branches, but if you think it's better this way I 
>>>>>>>>>>>>>>>> can change it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I would like something that tells me more clearly how 
>>>>>>>>>>>>>>> this new transition helper differs from the existing 
>>>>>>>>>>>>>>> TBIVM. Sharing the code between them and using different 
>>>>>>>>>>>>>>> args would be one way. Documenting the difference in 
>>>>>>>>>>>>>>> comments would be another. Your choice.
>>>>>>>>>>>>>> Ok, I added a description on top of TLBIVM.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Also I'm unclear what the "Lock" in 
>>>>>>>>>>>>>>>>> ThreadLockBlockInVM actually refers to. I find the 
>>>>>>>>>>>>>>>>> name quite jarring to read.
>>>>>>>>>>>>>>>> What about changing it to ThreadBlockinMonitor?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> That's not quite conveying the semantics. The problem is 
>>>>>>>>>>>>>>> that the semantics we are changing compared to TBIVM are 
>>>>>>>>>>>>>>> not evident in the TBIVM name. If TBIVM was actually 
>>>>>>>>>>>>>>> ThreadBlockInVMWithSafepointBlocking, then this new 
>>>>>>>>>>>>>>> transition would obviously be 
>>>>>>>>>>>>>>> ThreadBlockInVMWithoutSafepointBlocking - but perhaps 
>>>>>>>>>>>>>>> that lengthy, but clear name would be okay anyway?
>>>>>>>>>>>>>> Not convinced on that name since we are blocking at 
>>>>>>>>>>>>>> safepoints in the destructor. Based on the comments I 
>>>>>>>>>>>>>> added how about ThreadBlockinVMWithDeadlockPrevention ? 
>>>>>>>>>>>>>> or ThreadBlockinVMWithPreemption? (as in eliminate one of 
>>>>>>>>>>>>>> the conditions for deadlocks).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On the subject of naming, do_preempt and 
>>>>>>>>>>>>>>>>> preempt_by_safepoint don't really convey to me what 
>>>>>>>>>>>>>>>>> happens - what is being "preempted" here? I would 
>>>>>>>>>>>>>>>>> suggest a more direct Monitor::release_for_safepoint
>>>>>>>>>>>>>>>> Changed.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Logging: why "nativemonitor"? The logging in mutex.cpp 
>>>>>>>>>>>>>>>>> doesn't relate to a "native" monitor?? Actually I'm 
>>>>>>>>>>>>>>>>> not even sure if we need bother at all with the one 
>>>>>>>>>>>>>>>>> logging statement that is present.
>>>>>>>>>>>>>>>> I added it to eventually track unbounded try locks. Not 
>>>>>>>>>>>>>>>> sure I follow you with the name, isn't that how we name 
>>>>>>>>>>>>>>>> this monitors? I tried to differentiate them from Java 
>>>>>>>>>>>>>>>> monitors. What about just "monitor"?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> How about vmmonitor ?
>>>>>>>>>>>>>> Ok, changed.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> void Monitor::lock_without_safepoint_check(Thread * 
>>>>>>>>>>>>>>>>> self) {
>>>>>>>>>>>>>>>>>   // Ensure that the Monitor does not require or allow 
>>>>>>>>>>>>>>>>> safepoint checks.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The comment there should only say "not require".
>>>>>>>>>>>>>>>> Done.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> void Monitor::preempt_by_safepoint() {
>>>>>>>>>>>>>>>>>   _lock.unlock();
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Apart from renaming this as suggested above, aren't 
>>>>>>>>>>>>>>>>> there any suitable assertions we should have here? 
>>>>>>>>>>>>>>>>> safepoint-in-progress or handshake-in-progress? _owner 
>>>>>>>>>>>>>>>>> == Thread::current?
>>>>>>>>>>>>>>>> Ok, I added an assertion that owner should be NULL. 
>>>>>>>>>>>>>>>> Asserting safepoint-in-progress does not really work 
>>>>>>>>>>>>>>>> because _state could change to _not_synchronized right 
>>>>>>>>>>>>>>>> after you checked for it in TLBIVM.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Nit:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> assert(_owner == Thread::current(), "should be equal: 
>>>>>>>>>>>>>>>>> owner=" INTPTR_FORMAT
>>>>>>>>>>>>>>>>>                    ", self=" INTPTR_FORMAT, 
>>>>>>>>>>>>>>>>> p2i(_owner), p2i(Thread::current()));
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> with Dan's enhanced assertions there's an indentation 
>>>>>>>>>>>>>>>>> issue. The second line should indent to the first 
>>>>>>>>>>>>>>>>> comma, but that will make the second line extend way 
>>>>>>>>>>>>>>>>> past 80 columns.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Also you could factor that assertion for 
>>>>>>>>>>>>>>>>> _owner==Thread::current() into its own function or 
>>>>>>>>>>>>>>>>> macro to avoid the repetition.
>>>>>>>>>>>>>>>> Corrected indentation based on Dan's reply to align 
>>>>>>>>>>>>>>>> with _owner.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I though it should indent to the comma because it is a 
>>>>>>>>>>>>>>> continuation of the same argument being passed to the 
>>>>>>>>>>>>>>> assert "function". But I'm okay with Dan's suggestion.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Factoring it into its own little function or macro would 
>>>>>>>>>>>>>>> still be good to avoid the repetition.
>>>>>>>>>>>>>> Ok, added new function assert_owned_by_self(). I could 
>>>>>>>>>>>>>> change it to assert_owner(Thread*) and use it for the 
>>>>>>>>>>>>>> NULL case too unifying the printed messages to something 
>>>>>>>>>>>>>> like "invalid owner: owner=" INTPTR_FORMAT ", should be:" 
>>>>>>>>>>>>>> INTPTR_FORMAT. What do you think?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>  OSThreadWaitState osts(self->osthread(), false /* not 
>>>>>>>>>>>>>>>>> Object.wait() */);
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This needs to be returned to its original place as per 
>>>>>>>>>>>>>>>>> Dan's comments.
>>>>>>>>>>>>>>>> Done.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>     } else {
>>>>>>>>>>>>>>>>>       Monitor::lock(self);
>>>>>>>>>>>>>>>>>     }
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> You don't need Monitor:: here
>>>>>>>>>>>>>>>> Removed.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> // Temporary JVM_RawMonitor* support. A raw monitor 
>>>>>>>>>>>>>>>>> can just be a PlatformMonitor now.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This needs to be resolved before committing. Some of 
>>>>>>>>>>>>>>>>> the existing commentary on what raw monitors are needs 
>>>>>>>>>>>>>>>>> to be retained. Not clear if we need to set the _owner 
>>>>>>>>>>>>>>>>> field or can just skip it.
>>>>>>>>>>>>>>>> Is it okay if I keep the following comments?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> // Yet another degenerate version of Monitor::lock() or 
>>>>>>>>>>>>>>>> lock_without_safepoint_check()
>>>>>>>>>>>>>>>> // jvm_raw_lock() and _unlock() can be called by 
>>>>>>>>>>>>>>>> non-Java threads via JVM_RawMonitorEnter.
>>>>>>>>>>>>>>>> //
>>>>>>>>>>>>>>>> // There's no expectation that JVM_RawMonitors will 
>>>>>>>>>>>>>>>> interoperate properly with the native
>>>>>>>>>>>>>>>> // Mutex-Monitor constructs.  We happen to implement 
>>>>>>>>>>>>>>>> JVM_RawMonitors in terms of
>>>>>>>>>>>>>>>> // native Mutex-Monitors simply as a matter of 
>>>>>>>>>>>>>>>> convenience.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yep that's perfect. And as a future RFE we can replace 
>>>>>>>>>>>>>>> them with direct use of PlatformMonitor (or PlatformMutex).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I could keep setting the owner as _owner = 
>>>>>>>>>>>>>>>> Thread::current_or_null() in jvm_raw_lock(), at least 
>>>>>>>>>>>>>>>> it wouldn't hurt.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It's useful for checking usage errors, but we won't have 
>>>>>>>>>>>>>>> that if we replace with PlatformMonitor, so may as well 
>>>>>>>>>>>>>>> drop it now IMO.
>>>>>>>>>>>>>> Ok, I added asserts that _owner should be NULL after 
>>>>>>>>>>>>>> acquiring it and before releasing it though.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Monitor::~Monitor() {
>>>>>>>>>>>>>>>>>   assert(_owner == NULL, "should be NULL: owner=" 
>>>>>>>>>>>>>>>>> INTPTR_FORMAT, p2i(_owner));
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Will this automatically result in the PlatformMonitor 
>>>>>>>>>>>>>>>>> destructor being called?
>>>>>>>>>>>>>>>> Yes, should I add a comment to make it clear that 
>>>>>>>>>>>>>>>> ~PlatformMonitor() will be executed?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> No need - assume other people have a better 
>>>>>>>>>>>>>>> understanding of C++ than I do :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Below is version v04, which also contains a correction 
>>>>>>>>>>>>>> pointed out off-list by Robbin to do a local poll first 
>>>>>>>>>>>>>> in SafepointMechanism::callback_if_safepoint() if we are 
>>>>>>>>>>>>>> using local polls. Since the thread local poll is armed 
>>>>>>>>>>>>>> after changing _state to _synchronizing, if we only do a 
>>>>>>>>>>>>>> global poll in the TLBIVM constructor we could detect 
>>>>>>>>>>>>>> there is a safepoint in progress and callback but when 
>>>>>>>>>>>>>> coming back in the destructor 
>>>>>>>>>>>>>> SafepointMechanism::should_block() could miss detecting 
>>>>>>>>>>>>>> the safepoint in progress since that method checks first 
>>>>>>>>>>>>>> for local polls.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/
>>>>>>>>>>>>>> Inc: 
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v04/inc/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Patricio
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for looking into this! Waiting on your comments 
>>>>>>>>>>>>>>>> to send v04.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> Patricio
>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>



More information about the hotspot-dev mailing list