RFR [16] 8248170 Excessive include of compiledMethod, codeCache, javaClasses and systemDictionary

[coleen.phillimore at oracle.com]

On 6/24/20 5:41 AM, David Holmes wrote:
> On 24/06/2020 4:16 pm, Ioi Lam wrote:
>>
>>
>> On 6/23/20 9:08 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 24/06/2020 6:10 am, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8248170
>>>> http://cr.openjdk.java.net/~iklam/jdk16/8248170-fix-includes-compiledMethod-etc.v01/ 
>>>>
>>>>
>>>> {compiledMethod, codeCache, javaClasses, systemDictionary}.hpp are 
>>>> unnecessarily included in some popular headers. This introduces 
>>>> unnecessary dependencies and slows down compile time.
>>>
>>> Based on your non-PCH local builds?
>>>
>>> I have no issue with removing unnecessary includes but I see far 
>>> more added includes in your changes than I see removals, so I'm a 
>>> little confused by that. I also have to wonder how on earth you 
>>> concluded that these particular includes were the cause of your 
>>> problem ??
>>>
>> I ran a script on my local non-PCH build and got the following 
>> output. I scan the file from the bottom to look for things to fix.
>
> Interesting! Thanks for sharing.
>
>> http://cr.openjdk.java.net/~iklam/jdk16/misc/8248170-headers.txt
>>
>> The reasons for adding new includes: the fix has 2 parts
>>
>> (a) Remove unnecessary headers included by popular files such as 
>> thread.hpp
>> (b) The above causes some sources files to break, because they were 
>> missing includes. These files could compile before my patch because 
>> the missing dependency was transitively included by thread.hpp, etc. 
>> Now I need to add the missing includes.
>>
>> E.g., os_windows.cpp has this
>>
>> 2342     CodeBlob* cb = CodeCache::find_blob(pc);
>>
>> ... so it needs to include codeCache.hpp, but that was transitively 
>> included by vmOperations.hpp.
>
> Okay. Just seemed odd to see more added than taken away.
>
>> =============
>>
>> I updated the patch based on Coleen's off-line feedback:
>>
>> http://cr.openjdk.java.net/~iklam/jdk16/8248170-fix-includes-compiledMethod-etc.v02/ 
>>
>> http://cr.openjdk.java.net/~iklam/jdk16/8248170-fix-includes-compiledMethod-etc.v02.delta/ 
>
>
> Appears to me that c1_CodeStubs.hpp includes c1/c1_Runtime1.hpp, and 
> each of the c1_CodeStubs_<arch>.cpp files also include it. That seems 
> redundant.

I don't immediately see where c1_CodeStubs.hpp needs c1_Runtime1.hpp, so 
maybe it doesn't need it, and the #include can be pushed down to the 
.cpp files that might not already transitively include it.   If it's a 
small change, you can include it with this, but honestly, there could be 
many iterations of cleaning up our includes, so it can be done in a 
future round also.

Thanks,
Coleen
>
> Thanks,
> David
> -----
>
>>
>>
>> Thanks
>> - Ioi
>>
>>> The change seems harmless so I don't object.
>>>
>>> Thanks,
>>> David
>>>
>>>> slowdebug:
>>>> total number of files compiled: 264606 -> 260588 = -1.5%
>>>> total number of lines compiled: 76073364 -> 74477347 = -2.1%
>>>> size of libjvm.so (internal symbols): 234180296 bytes -> 233129904 
>>>> bytes = -0.5%
>>>>
>>>> (I got the statistics by parsing the *.d files in the build 
>>>> directory).
>>>>
>>>> Thanks
>>>> - Ioi
>>