RFR(s): 8214181: safepoint header cleanup

Robbin Ehn robbin.ehn at oracle.com
Thu Nov 22 10:29:01 UTC 2018


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


More information about the hotspot-runtime-dev mailing list