RFR(M) 8252526 Remove excessive inclusion of jvmti.h and jvmtiExport.hpp

[Ioi Lam]

On 8/31/20 6:17 PM, Ioi Lam wrote:
>
>
> On 8/31/20 4:05 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> I haven't looked at the code changes ...
>>
>> On 1/09/2020 4:13 am, Ioi Lam wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8252526
>>> http://cr.openjdk.java.net/~iklam/jdk16/8252526-fix-jvmti-hpp.v01/
>>>
>>> (I marked this RFR as "M" because 63 files have changed, but most of 
>>> the are
>>> just adding a missing #include "prims/jvmtiExport.hpp").
>>>
>>> jvmti.h is included 905 times and jvmtiExport.hpp is included 776 times
>>> (by 971 hotspot .o files). Most of these are unnecessarily included 
>>> by the
>>> following 3 popular header files:
>>>
>>> [1] javaClasses.hpp: ThreadStatus is rarely used, and should be moved
>>>      to javaThreadStatus.hpp. I also converted the enum to an C++ 11
>>>      enum class for better type safety. (see also JDK-8247938).
>>>
>>> [2] os.hpp: No need to include jvm.h. Use forward declaration
>>>      "typedef struct _jvmtiTimerInfo jvmtiTimerInfo;" instead.
>>
>> That does not seem reasonable to me. It is one thing to do a simple 
>> forward declaration of a class but this is an internal detail of 
>> JVMTI which os.hpp has no business knowing about.
>>
>
> How about changing jvmti.h from:
>
> struct _jvmtiTimerInfo;
> typedef struct _jvmtiTimerInfo jvmtiTimerInfo;
>
> to
>
> struct jvmtiTimerInfo;
> typedef struct jvmtiTimerInfo jvmtiTimerInfo;
>
> Then os.hpp can declare:
>
> struct jvmtiTimerInfo;
>
> I wonder why we use the _ prefix. It seems like an anachronism to me. 
> Do we still support C compilers that cannot handle "typedef struct Foo 
> Foo;"
>
In fact I think this typedef is not an implementation detail. It's not 
intended to hide anything in jvmti.h. I.e., it's not a typedef of an 
opaque structure. All fields in the structure can be accessed without 
any access control.

It's just an odd style of declaring a structure that tries to be 
compatible with ancient C compilers, so you can use "jvmtiTimerInfo" 
instead of "struct jvmtiTimerInfo" everywhere. It could have been 
written like this (from our official spec):

https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html

typedef struct {
     jlong max_value;
     jboolean may_skip_forward;
     jboolean may_skip_backward;
     jvmtiTimerKind kind;
     jlong reserved1;
     jlong reserved2;
} jvmtiTimerInfo;

Why don't we use this style in jvmti.h? Well, one difference is that 
this style cannot be forward declared without revealing the contents of 
the struct!

I would argue that the reason for the current style is for the *very 
purpose* so that it can be forward declared :-)

Thanks
- Ioi


>>> [3] thread.hpp: No need to include jvmExport.hpp. Use forward 
>>> declaration
>>>      for JvmtiSampledObjectAllocEventCollector and
>>>      JvmtiVMObjectAllocEventCollector instead.
>>>
>>>
>>> The total number of includes have reduced from 252033 to 250001. 
>>> Build time of
>>> slow-debug libjvm.so is reduced from 2:07 to 2:04 on my machine.
>>
>> I'm not sure why we really care. The build times are not that 
>> problematic that it warrants some of these header file hacks IMO. 
>
> My main goal is to speed up incremental builds. Currently we have more 
> than 200 header files that are included by more than half of the .o 
> files of hotspot. I would often get massive rebuilds after touching a 
> header file. Removing the unnecessary dependencies will help developer 
> productivity.
>
> Also, currently we do a lot of build validations in tier5 and we had 
> consistently missed build problems with the minimal build. I suggested 
> to move those builds to tier1, but that was turned down because the 
> builds are not fast enough.
>
>> Are those figures with or without PCH?
>>
> I tested on Linux without PCH.
>
> Thanks
> - Ioi
>
>> David
>> -----
>>
>>>
>>> Thanks
>>> - Ioi
>>>
>