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

[David Holmes]

On 1/09/2020 2:00 pm, Ioi Lam wrote:
> 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;

Does that actually work? If so that's equivalent to "class Foo;" forward 
declarations and so is acceptable.

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

I don't agree with the conclusion. ;-)

The intent is that anyone who needs jvmtiTimerInfo #includes jvmti.h - 
that is the whole point of header files afterall. os.hpp has no business 
knowing what jvmtiTimerInfo needs to be typedef'd to to make things work.

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

On some absolute scale of measurement perhaps, but in practice people 
tend to handle other tasks while waiting for builds to complete.

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

But they are full builds, not incremental so I don't see how this work 
would really help them. Especially as ...

>>
>>> Are those figures with or without PCH?
>>>
>> I tested on Linux without PCH.

... most builds use PCH so we aren't recompiling these header files 
unnecessarily.

I'm not objecting to doing the cleanup in this area as long as we aren't 
overly contorting things just to avoid an include that logically is 
actually needed. The initial jvmtiTimerInfo situation is on the wrong 
side of the line IMO - and unfortunately, due to the way the os class is 
declared, it isn't one easily fixed by refactoring.

Cheers,
David
-----

>>
>> Thanks
>> - Ioi
>>
>>> David
>>> -----
>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>
>