RFR(s): 8214181: safepoint header cleanup

[Daniel D. Daugherty]

No worries. Just closing the loop since I reviewed a few of the
early versions of the collection... :-)

Dan


On 11/24/18 3:33 PM, Robbin Ehn wrote:
> Thanks Dan, unfortunate it seem like I was a bit trigger happy during 
> thanks giving and already pushed. Lucky you gave me thumb up :)
>
> On 2018-11-24 15:10, Daniel D. Daugherty wrote:
>>  > http://cr.openjdk.java.net/~rehn/8214181/v2/webrev/
>>
>> src/hotspot/share/gc/g1/g1HeapVerifier.cpp
>>      No comments.
>>
>> src/hotspot/share/memory/iterator.cpp
>>      No comments.
>>
>> src/hotspot/share/runtime/safepoint.hpp
>>      No comments.
>>
>> src/hotspot/share/runtime/thread.hpp
>>      No comments.
>>
>> src/hotspot/share/runtime/thread.inline.hpp
>>      No comments.
>>
>> src/hotspot/share/services/threadService.hpp
>>      No comments.
>>
>> src/hotspot/share/utilities/hashtable.cpp
>>      No comments.
>>
>> Thumbs up.
>>
>> Dan
>>
>>
>> On 11/22/18 5:29 AM, Robbin Ehn wrote:
>>> Hi David,
>>>
>>> On 11/22/18 4:04 AM, David Holmes wrote:
>>>> Hi Robbin,
>>>>
>>>> On 22/11/2018 12:17 am, Robbin Ehn wrote:
>>>>> Hi all, please review.
>>>>>
>>>>> A cleanup of unused headers in safepoint.hpp, which have some 
>>>>> minor collateral effects.
>>>>
>>>> I see a lot of changes here which seem unrelated to cleaning up 
>>>> safepoint.hpp per-se. The * changes are a distraction.
>>>
>>> They are *semi* related, when I looked the forward decl of
>>> ThreadSafepointState I saw it. And Coleen like that.
>>>
>>>>
>>>> src/hotspot/share/gc/g1/g1HeapVerifier.cpp
>>>>
>>>> I see no include of safepoint.hpp that would have previously caused 
>>>> code/nmethod.hpp to be included - what was the path here?
>>>>
>>>> Ditto for src/hotspot/share/memory/iterator.cpp
>>>>
>>>> ---
>>>> src/hotspot/share/runtime/thread.hpp
>>>>
>>>> How does code/compiledMethod.hpp relate to safepoint.hpp?
>>>
>>> So the reason for changing above is that they include thread.hpp via 
>>> other
>>> headers and got safepoint.hpp which gave them nmethod/compiledMethod.
>>>
>>>>
>>>> Moving stuff to .inline.hpp is fine, but seems unrelated to 
>>>> safepoint header cleanup.
>>>
>>> I split out these header changes out from a bigger patch, they are 
>>> needed for that patch. thread.hpp do not longer include safepoint.hpp.
>>>
>>>>
>>>> With the header file changes I have to wonder how many of those 
>>>> advance class definitions are actually needed. The
>>>>
>>>>   61 class ThreadSafepointState;
>>>>
>>>> wasn't needed before, but now it is as you've removed the include.
>>>
>>> I removed SnippetCache and nmethod but added JavaThread.
>>>
>>>>
>>>> ---
>>>>
>>>> safepoint.hpp:
>>>>
>>>> I don't think I'll ever figure out how include headers are supposed 
>>>> to be handled. I see so many types being used in safepoint.hpp for 
>>>> which there is no include for the header defining those types! :(
>>>>
>>>> That said I don't think we should be making implicit dependencies 
>>>> worse, so as safepoint.hpp calls os::javaTimeMillis() I think the 
>>>> os.hpp include should remain.
>>>
>>> Yes, my bad, added back os.hpp.
>>> And I added utilities/globalDefinitions.hpp for address and 
>>> JavaThreadState.
>>>
>>> Here is inc:
>>> http://cr.openjdk.java.net/~rehn/8214181/v2/inc/webrev/index.html
>>> Full:
>>> http://cr.openjdk.java.net/~rehn/8214181/v2/webrev/
>>>
>>> Build locally without precompiled and tier 1.
>>>
>>> Thanks, Robbin
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8214181
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~rehn/8214181/webrev/index.html
>>>>> Passes t1, locally built without precompiled headers.
>>>>>
>>>>> Thanks, Robbin
>>