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

[Chris Plummer]

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.

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
>>>>
>>>>
>>>
>>>