RFR(s): 8214181: safepoint header cleanup

[David Holmes]

On 22/11/2018 8:29 pm, 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/

Looks good.

Thanks,
David
-----

> 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