RFR(s): 8214181: safepoint header cleanup

[Robbin Ehn]

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
>