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

Chris Plummer chris.plummer at oracle.com
Wed Mar 21 17:30:53 UTC 2018


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




More information about the hotspot-runtime-dev mailing list