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

[David Holmes]

On 1/09/2020 11:34 pm, Daniel D. Daugherty wrote:
> On 9/1/20 9:24 AM, Ioi Lam wrote:
>>
>>
>> On 8/31/20 9:38 PM, David Holmes wrote:
>>> 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.
>> Yes, it works, but I would need to file a CSR because jvmti.h is 
>> included in the JDK ....
> 
> Yes, jvmti.h is included in the JDK, but the name exposed by the
> spec is 'jvmtiTimerInfo'. The '_jvmtiTimerInfo' name isn't exposed by
> the spec at all.
> 
> You could change the name to 'DO_NOT_USE_THIS_NAME_jvmtiTimerInfo' and
> nothing should break from a JVM/TI spec POV. Of course, if there is code
> out in the world that depended on that name, then it will break, but I
> would argue that code is broken.
> 
> Short version: I think '_jvmtiTimerInfo' is an implementation detail
> and you don't need a CSR for correctness. You might want a CSR for
> advice since this is an odd situation, but, strictly from an API POV,
> I don't think you need one.

I agree no CSR needed this does not affect any exported API, nor is 
there a behaviour change via an exported API.

David
-----

> 
> Dan
> 
> 
>>
>> Thanks
>> - Ioi
>>
>> =======
>> $ hg diff
>> diff -r aaa4245df83a src/hotspot/share/prims/jvmtiH.xsl
>> --- a/src/hotspot/share/prims/jvmtiH.xsl    Mon Aug 31 11:03:13 2020 
>> -0700
>> +++ b/src/hotspot/share/prims/jvmtiH.xsl    Mon Aug 31 23:24:27 2020 
>> -0700
>> @@ -406,11 +406,11 @@
>>  </xsl:template>
>>
>>  <xsl:template match="typedef" mode="early">
>> -  <xsl:text>struct _</xsl:text>
>> +  <xsl:text>struct </xsl:text>
>>    <xsl:value-of select="@id"/>
>>    <xsl:text>;
>>  </xsl:text>
>> -  <xsl:text>typedef struct _</xsl:text>
>> +  <xsl:text>typedef struct </xsl:text>
>>    <xsl:value-of select="@id"/>
>>    <xsl:text> </xsl:text>
>>    <xsl:value-of select="@id"/>
>> @@ -419,7 +419,7 @@
>>  </xsl:template>
>>
>>  <xsl:template match="typedef" mode="body">
>> -  <xsl:text>struct _</xsl:text>
>> +  <xsl:text>struct </xsl:text>
>>    <xsl:value-of select="@id"/>
>>    <xsl:text> {
>>  </xsl:text>
>> diff -r aaa4245df83a src/hotspot/share/runtime/os.hpp
>> --- a/src/hotspot/share/runtime/os.hpp    Mon Aug 31 11:03:13 2020 -0700
>> +++ b/src/hotspot/share/runtime/os.hpp    Mon Aug 31 23:24:27 2020 -0700
>> @@ -51,7 +51,7 @@
>>  class OSThread;
>>  class Mutex;
>>
>> -typedef struct _jvmtiTimerInfo jvmtiTimerInfo;
>> +struct jvmtiTimerInfo;
>>
>>  template<class E> class GrowableArray;
>> =======
>>
>>
>>
>>
>>
>>
>>
>