RFR(S): 8195109: ServiceUtil::visible_oop is not needed anymore

[Stefan Karlsson]

Looks good.

StefanK

On 2018-03-21 18:30, Chris Plummer wrote:
> On 3/21/18 8:56 AM, Chris Plummer wrote:
>> On 3/21/18 3:29 AM, Stefan Karlsson wrote:
>>> On 2018-03-20 23:15, JC Beyler wrote:
>>>> Hi Chris,
>>>>
>>>> I saw a nit in jvmtiExport.cpp, you seem to have removed the wrong 
>>>> '}' in
>>>> ~JvmtiVMObjectAllocEventCollector. Line 2767 was removed and not 
>>>> 2766, no?
>>>> It doesn't matter for compilation/functionality but is wrong 
>>>> indentation
>>>> wise; hence the nit :)
>>>
>>> I think this webrev was generated without the -b flag, and hence 
>>> doesn't always show the right whitespace changes.
>>>
>>> If you look in the patch file:
>>> http://cr.openjdk.java.net/~cjplummer/8195109/webrev.00/src/hotspot/share/prims/jvmtiExport.cpp.patch 
>>>
>>>
>>> it looks correct:
>>> -      if (ServiceUtil::visible_oop(obj)) {
>>> - JvmtiExport::post_vm_object_alloc(JavaThread::current(), obj);
>>> -      }
>>> +      JvmtiExport::post_vm_object_alloc(JavaThread::current(), obj);
>>>      }
>>>
>>> Cheers,
>>> StefanK
>> I didn't know about the -b option. Seems like that should be the default.
>>
>> I doubled checked the source and it is correct.
>>
>> Thanks for the reviews.
> Well, I thought I had addressed the solaris-sparc compilation issues 
> yesterday because the test build I submitted passed, but today I got 
> another error when I did some sanity testing before push. Turns out I 
> tested the wrong repo. threadSMR.cpp needs another #include:
> 
> diff --git a/src/hotspot/share/runtime/threadSMR.cpp 
> b/src/hotspot/share/runtime/threadSMR.cpp
> --- a/src/hotspot/share/runtime/threadSMR.cpp
> +++ b/src/hotspot/share/runtime/threadSMR.cpp
> @@ -29,8 +29,10 @@
>   #include "runtime/thread.inline.hpp"
>   #include "runtime/threadSMR.inline.hpp"
>   #include "services/threadService.hpp"
> +#include "utilities/copy.hpp"
>   #include "utilities/globalDefinitions.hpp"
>   #include "utilities/resourceHash.hpp"
> +#include "utilities/vmError.hpp"
> 
>   Monitor*              ThreadsSMRSupport::_delete_lock =
>                             new Monitor(Monitor::special, 
> "Thread_SMR_delete_lock",
> 
> thanks,
> 
> Chris
>>
>> Chris
>>>
>>>>
>>>> I only noticed because I've playing around with that exact code and 
>>>> this
>>>> will simplify my own webrev once this goes in!
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>>
>>>> On Tue, Mar 20, 2018 at 1:08 PM <coleen.phillimore at oracle.com> wrote:
>>>>
>>>>>
>>>>>
>>>>> On 3/20/18 3:39 PM, Chris Plummer wrote:
>>>>>> Hi,
>>>>>>
>>>>>> New webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~cjplummer/8195109/webrev.01/index.html
>>>>>>
>>>>>> There was a build failure on solaris-sparc in threadSMR.cpp.
>>>>>> References to the Copy class were producing "unresolved symbol"
>>>>>> errors. threadSMR.cpp includes threadService.hpp, which no longer
>>>>>> includes serviceUtil.hpp (because it was removed). It looks like
>>>>>> serviceUtil.hpp indirectly included "utilities/copy.hpp", so now I
>>>>>> include it directly in threadSMR.cpp. The problem was only on
>>>>>> solaris-sparc, so I assume on other platforms there was platform
>>>>>> dependent code indirectly pulling in copy.hpp. In any case, it's now
>>>>>> directly pulled in on all platforms.
>>>>>
>>>>> Yes, this looks good!
>>>>> Coleen
>>>>>
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/19/18 5:48 PM, Chris Plummer wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review the following:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8195109
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8195109/webrev.00/index.html
>>>>>>>
>>>>>>> The assert I added to make sure this is safe has been in place in
>>>>>>> jdk/jdk for almost 3 weeks with no issues (longer in jdk/hs).
>>>>>>>
>>>>>>> The webrev is missing the copyright update for threadService.hpp. I
>>>>>>> fixed it after noticing that.
>>>>>>>
>>>>>>> Testing is in progress. Running hs tiers 1, 2, and 3, and jdk 
>>>>>>> tiers 1
>>>>>>> and 2. Also making sure all serviceability tests are run.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>
>>>>>>
>>>>>
>>>>>
>>
>>
> 
>