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

[Daniel D. Daugherty]

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.

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