Hi Jc,
Okay, you convinced me - thanks!
Added new line into the generated jvmti.h:
#define JVMTI_VERSION_LATEST JVMTI_VERSION
I hope, it will help in your case.
Updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8219023-svc-version.2/
This version includes suggestions from David.
Thanks,
Serguei
On 5/9/19 14:17, Jean Christophe Beyler wrote:
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
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
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, [email protected]
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, [email protected]
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
>>
--
--
|
