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