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

[Ioi Lam]

On 6/24/20 5:49 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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.
>
Hi Coleen, thanks for the review.

With the current patch, c1_CodeStubs.hpp is included only 15 times, so 
even if it has unnecessary includes, fixing them would not gain much. 
I'll leave it alone for now.

Thanks
- Ioi

> 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
>>>
>