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

[Stefan Karlsson]

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