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

[David Holmes]

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.

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
>