RFR (S): 8219023: Investigate syncing JVMTI spec version with JDK version
Jean Christophe Beyler
jcbeyler at google.com
Thu May 9 21:17:07 UTC 2019
Hi Serguei,
Of course I can :)
Consider, just randomly of course, the heap sampling work that got added to
JVMTI. Now imagine you'd want to test if it is supported by a given JVMTI
version, you would write something like this:
bool HeapMonitor::Supported(jvmtiEnv *jvmti) {
jvmtiCapabilities caps;
memset(&caps, 0, sizeof(caps));
if (jvmti->GetPotentialCapabilities(&caps) != JVMTI_ERROR_NONE) {
LOG(WARNING) << "Failed to get potential capabilities, disabling the
heap "
<< "sampling monitor";
return false;
}
return caps.can_generate_sampled_object_alloc_events
&& caps.can_generate_garbage_collection_events;
}
Now, the problem is that this code cannot be used if you compile with an
older JDK such as JDK8 for example
because can_generate_sampled_object_alloc_events does not exist yet for
that jvmti.h file.
In a perfect world, we might imagine that we are always compiling with the
latest JVMTI headers but that is not always true and, therefore, to have
the code portable, we now have to do:
bool HeapMonitor::Supported(jvmtiEnv *jvmti) {
#ifdef ENABLE_HEAP_SAMPLING
jvmtiCapabilities caps;
memset(&caps, 0, sizeof(caps));
if (jvmti->GetPotentialCapabilities(&caps) != JVMTI_ERROR_NONE) {
LOG(WARNING) << "Failed to get potential capabilities, disabling the
heap "
<< "sampling monitor";
return false;
}
return caps.can_generate_sampled_object_alloc_events
&& caps.can_generate_garbage_collection_events;
#else
return false;
#endif
}
Where ENABLE_HEAP_SAMPLING is defined if we did compile with JDK11 and not
defined if we compiled with JDK8. I can't use JVMTI_VERSION because I can't
use it in an #if because it is not an enum. Were it to be a #define or were
I to have a #define I could use with a version number being bumped up, I
could write something such as:
#ifdef JVMTI_VERSION_11
or something that looks in the value of the JVMTI_VERSION if I wanted.
Right now, I can't even do that!
Hopefully this helps understand what I am talking about :-),
Jc
On Thu, May 9, 2019 at 2:08 PM serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> wrote:
> Hi Jc,
>
> Thank you a lot for review!
> Some replies below.
>
>
> On 5/9/19 09:10, Jean Christophe Beyler wrote:
>
> Hi Serguei,
>
> FWIW, the change looks good and I think it's a good idea to do. However,
> there is one thorn in our internal agent code is that the JVMTI_VERSION is
> in an enum. This makes us unable to #if it when adding usages of newer
> features/methods.
>
> This probably could/should be a different webrev (which I can do if you
> like) but is there any way while you are changing this that the enum for
> JVMTI_VERSION could become a set of #define?
>
> So instead of:
> enum {
> JVMTI_VERSION_1 = 0x30010000,
> JVMTI_VERSION_1_0 = 0x30010000,
> JVMTI_VERSION_1_1 = 0x30010100,
> JVMTI_VERSION_1_2 = 0x30010200,
> JVMTI_VERSION_9 = 0x30090000,
> JVMTI_VERSION_11 = 0x300B0000,
>
> JVMTI_VERSION = 0x30000000 + (11 * 0x10000) + (0 * 0x100) + 0 /*
> version: 11.0.0 */
> };
>
> We would get:
> #define JVMTI_VERSION_1 0x30010000
> #define JVMTI_VERSION_1_0 0x30010000
> #define JVMTI_VERSION_1_1 = 0x30010100
> #define JVMTI_VERSION_1_2 = 0x30010200
> #define JVMTI_VERSION_9 = 0x30090000
> #define JVMTI_VERSION_11 = 0x300B0000
> #define JVMTI_VERSION (0x30000000 + (11 * 0x10000) + (0 * 0x100) + 0 /*
> version: 11.0.0 */)
>
>
> It is interesting concern and suggestion.
> I'm not sure if it requires a CSR.
>
>
> I actually don't care about any define of these except for JVMTI_VERSION;
> basically it would be useful so that in our agent code we can test the
> JVMTI_VERSION with #if macros to protect the code when new elements show up
> in future versions. So it also could be:
> enum {
> JVMTI_VERSION_1 = 0x30010000,
> JVMTI_VERSION_1_0 = 0x30010000,
> JVMTI_VERSION_1_1 = 0x30010100,
> JVMTI_VERSION_1_2 = 0x30010200,
> JVMTI_VERSION_9 = 0x30090000,
> JVMTI_VERSION_11 = 0x300B0000,
>
> JVMTI_VERSION = 0x30000000 + (11 * 0x10000) + (0 * 0x100) + 0 /*
> version: 11.0.0 */
> };
>
> #define JVMTI_LATEST_VERSION (0x30000000 + (11 * 0x10000) + (0 * 0x100) +
> 0 /* version: 11.0.0 */)
>
>
> I is not a problem to implement this one.
> But I'm not sure how does this really help in your case.
> I do not see a point to test the JVMTI_VERSION with #if as it is always
> defined.
> Could you, please, elaborate a little bit more?
>
> Thanks,
> Serguei
>
> Right now, I have to do weird things where I detect the jvmti.h used at
> compile time to then do -DUSING_JDK11 for the agent at compile time.
>
> Thanks!
> Jc
>
>
>
>
> On Thu, May 9, 2019 at 8:48 AM serguei.spitsyn at oracle.com <
> serguei.spitsyn at oracle.com> wrote:
>
>> I'll try to get rid of VERSION_INTERIM.
>> Always using just VERSION_FEATURE.0.0 should not create problems
>> if we do not change JVMTI spec in VERSION_UPDATE.
>> I do not see why we would change the JVMTI spec in update releases.
>> But if we do then using VERSION_UPDATE as microversion would be good
>> enough.
>>
>> Thanks!
>> Serguei
>>
>>
>> On 5/9/19 06:13, David Holmes wrote:
>> > Hi Serguei,
>> >
>> > On 9/05/2019 7:09 pm, serguei.spitsyn at oracle.com wrote:
>> >> Hi David,
>> >>
>> >> Thank you a lot for review!
>> >> There are some replies below.
>> >>
>> >>
>> >> On 5/8/19 18:42, David Holmes wrote:
>> >>> Hi Serguei,
>> >>>
>> >>> On 9/05/2019 8:57 am, serguei.spitsyn at oracle.com wrote:
>> >>>> Please, review a fix for the task:
>> >>>> https://bugs.openjdk.java.net/browse/JDK-8219023
>> >>>>
>> >>>> Webrev:
>> >>>>
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8219023-svc-version.1/
>> >>>>
>> >>>>
>> >>>> Summary:
>> >>>>
>> >>>> By design as we have never bumped the JVMTI version unless
>> >>>> there were spec changes for that release. Now we want to sync
>> >>>> the JVMTI version with the JDK version regardless of whether
>> >>>> or not spec changes have occurred in that release.
>> >>>> Also, we want it automatically set by the build system so that
>> >>>> no manual updates are needed for each release.
>> >>>>
>> >>>> The jvmti.h and jvmti.html (JVMTI spec) are generated from
>> >>>> the jvmti.xsl with the XSLT scripts now. So, the fix removes
>> >>>> hard coded major, minor and micro versions from the jvmti.xml
>> >>>> and passes major and minor parameters with the -PARAMETER
>> >>>> to the XSL transformation.
>> >>>>
>> >>>> Another part of the fix is in the JDI which starts using JDK
>> >>>> versions now instead of maintaining its own, and in the JDWP
>> >>>> agent which is using the JVMTI versions instead of its own.
>> >>>
>> >>> This all seems reasonable (though I'm no expert on working with XSL
>> >>> etc).
>> >>>
>> >>> One thing I am unclear of is why you bother with using
>> >>> VERSION_INTERIM when the actual version check will only consider
>> >>> VERSION_FEATURE (aka major). Couldn't you just leave the "minor"
>> >>> part 0 the same as the "micro" part?
>> >>
>> >> This is right question to ask.
>> >> I was two-folded on this.
>> >> But finally decided to maintain minor version (aka VERSION_INTERIM).
>> >> Then the JVMTI and debugger version will match the VM and JDK version
>> >> for update releases.
>> >> If understand it correctly, we are still going to have major.minor
>> >> versions.
>> >
>> > Not really. What we have now are things like 11.0.3 and 12.0.1 - only
>> > using the first and third parts. The full 4 part version string is:
>> >
>> > $VERSION_FEATURE.$VERSION_INTERIM.$VERSION_UPDATE.$VERSION_PATCH
>> >
>> > and we typically only update version_feature and version_update.
>> >
>> > https://openjdk.java.net/jeps/322
>> >
>> >> Also, the JVMTI GetVersionNumberspec still tells about both minor and
>> >> micro versions.
>> >> It also defines special constants for corresponding masks and shifts:
>> >>
>> >> Version Masks
>> >> Constant Value Description
>> >> |JVMTI_VERSION_MASK_INTERFACE_TYPE| 0x70000000 Mask to
>> >> extract
>> >> interface type. The value of the version returned by this function
>> >> masked with |JVMTI_VERSION_MASK_INTERFACE_TYPE| is always
>> >> |JVMTI_VERSION_INTERFACE_JVMTI| since this is a JVMTI function.
>> >> |JVMTI_VERSION_MASK_MAJOR| 0x0FFF0000 Mask to extract major
>> >> version number.
>> >> |JVMTI_VERSION_MASK_MINOR| 0x0000FF00 Mask to extract minor
>> >> version number.
>> >> |JVMTI_VERSION_MASK_MICRO| 0x000000FF Mask to extract micro
>> >> version number.
>> >>
>> >> Version Shifts
>> >> Constant Value Description
>> >> |JVMTI_VERSION_SHIFT_MAJOR| 16 Shift to extract major
>> >> version number.
>> >> |JVMTI_VERSION_SHIFT_MINOR| 8 Shift to extract minor
>> >> version number.
>> >> |JVMTI_VERSION_SHIFT_MICRO| 0 Shift to extract micro
>> >> version number.
>> >>
>> >>
>> >> This is link to the spec:
>> >>
>> https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#GetVersionNumber
>> >>
>> >>
>> >> It seems, changing (and/or deprecating) this will give more problems
>> >> than benefits.
>> >> It is better to remain compatible with previous releases.
>> >
>> > This is a problem that was flagged when the new versioning scheme was
>> > introduced but I'm guessing nothing was actually done about it. They
>> > are not really compatible beyond the major/feature part.
>> >
>> > If we only update the spec version with the feature version then all
>> > versions will have the form N.0.0. However your changes will also
>> > update if we happen to use a VERSION_INTERIM for some reason - though
>> > the version check will ignore that anyway. I'm not really seeing the
>> > point in having that happen.
>> >
>> > Maybe we do need to define a new version API that maps to the new
>> > versioning scheme of OpenJDK ? But if we did that we'd still have to
>> > support the legacy mapping and I'd still advocate simply using
>> > VERSION_FEATURE.0.0.
>> >
>> > It's tricky.
>> >
>> > David
>> > -----
>> >
>> >>> For the record I considered whether this needs a CSR request and
>> >>> concluded it did not as it doesn't involve changing any actual
>> >>> specifications.
>> >>
>> >> Okay, thanks.
>> >> I considered it too, made the same conclusion but still have some
>> >> doubt. :)
>> >>
>> >> Thanks!
>> >> Serguei
>> >>
>> >>>
>> >>> Thanks,
>> >>> David
>> >>>
>> >>>> Testing:
>> >>>> Generated docs and jvmti.h and checked the versions are correct.
>> >>>>
>> >>>> One could ask if we have to use same or similar approach for
>> >>>> other API's and tools, like JNI, JMX and so on.
>> >>>> But these are not areas of my expertise or responsibility.
>> >>>> It just feels like there is some room for unification here.
>> >>>>
>> >>>> Thanks,
>> >>>> Serguei
>> >>
>>
>>
>
> --
>
> Thanks,
> Jc
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190509/1b3f6a45/attachment-0001.html>
More information about the serviceability-dev
mailing list