Re: RFR(XS): 8066814: Reduce accessibility in TraceEvent
Looks good! Thanks, /Staffan On 17 dec 2014, at 15:45, Markus Grönlund markus.gronl...@oracle.com wrote: Greetings, Kindly asking for reviews for the following changeset: Bug: https://bugs.openjdk.java.net/browse/JDK-8066814 https://bugs.openjdk.java.net/browse/JDK-8066814 Webrev: http://cr.openjdk.java.net/~mgronlun/8066814/webrev01/ http://cr.openjdk.java.net/~mgronlun/8066814/webrev01/ Description: TraceEvent currently exposes internals unnecessarily. Therefore: Remove unnecessarily exposed methods. Add assert for not committing a cancelled event. Add method stubs for !INCLUDE_TRACE Thanks in advance Markus
Re: RFR(S): backport 8039995 and 8061785 changes to SA testcase
Looks good! Thanks, /Staffan On 17 dec 2014, at 17:15, KEVIN WALLS kevin.wa...@oracle.com wrote: Hi, I'd like a review of a backport of these test changes, into 8u. webrev: http://cr.openjdk.java.net/~kevinw/8039995_8061785/webrev.00/ Changes simply hg imported from: 8039995 http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/raw-rev/da92e4c42b24 8061785 http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/raw-rev/3cdb9f480a8c Thanks! Kevin
Re: RFR(S): backport 8039995 and 8061785 changes to SA testcase
Thanks! On 18/12/2014 08:12, Staffan Larsen wrote: Looks good! Thanks, /Staffan On 17 dec 2014, at 17:15, KEVIN WALLS kevin.wa...@oracle.com wrote: Hi, I'd like a review of a backport of these test changes, into 8u. webrev: http://cr.openjdk.java.net/~kevinw/8039995_8061785/webrev.00/
Re: RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Great Staffan! Thanks a lot, Volker On Thu, Dec 18, 2014 at 10:31 AM, Staffan Larsen staffan.lar...@oracle.com wrote: Since both Volker and Dmitry have reviewed this, no further reviews are necessary. I took a look at the changes in shared code, anyway, and it looks good. Thanks, /Staffan On 17 dec 2014, at 19:06, Dmitry Samersoff dmitry.samers...@oracle.com wrote: Volker, The changes looks good for me and I'll sponsor the push. But please check, whether you need one more reviewer or not. -Dmitry On 2014-12-17 20:37, Volker Simonis wrote: Hi Dmitry, once again, thanks for your detailed review. You can find the new version of the webrev under: http://cr.openjdk.java.net/~simonis/webrevs/8049716.v2/ I've rebased it to the latest jdk9/hs-rt repository today. I hope I adressed all your concerns. Please find my additional comments inline below: And I still need a sponsor for pushing this reviewed change. Any volunteers:) Thank you and best regards, Volker Below is fist part of review - shared files. * agent/make/Makefile - no comments * agent/src/os/linux/LinuxDebuggerLocal.c - no comments * agent/src/os/linux/symtab.c: 438: - What is the magic of symbols that starts with '.' ? - As far as I understand you are getting indirect value from opd section. There's already a very detailed descrition of the whole story in hotspot/src/share/vm/utilities/elfFuncDescTable.hpp and I've added a reference to that file in the comment now. Could you reformat it a bit to have it better readable? Something like: uintptr_t sym_value; ... symvalue = syms-st_value #ifdef ppc64 // Some comments here // ppc specific code here sym_value = #endif symtab-symbols[j].offset = sym_value - baseaddr; Good point. Done as suggested by you. 454: I appreciate detailed comments here. if (false) can cause unreachable code warning, and unused variable warning so it might be better to add #ifdef ppc64 on caller site at ll. 516 and leave here only a comment. But if you decide to guard against try_debuginfo please replace if (false) to goto quit I think there's already a comment in place. The problem is that the debuginfo file only has an empty .opd section. So if we would use the debuginfo file for symbol resolution we would still need the .opd section from the regular library. This doesn't fit in the current function work flow which only uses ELF data from a single file and I didn’t wanted to change that. But your suggestion of using 'goto quit' is good and I've used that version. * agent/src/share/classes/sun/jvm/hotspot/debugger/MachineDescriptionPPC64.java 38: return (endian.equals(big)); is enough I've used the even shorter version proposed by Alexander Smundak: return big.equals(System.getProperty(sun.cpu.endian)) * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxCDebugger.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThreadContextFactory.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ProcDebuggerLocal.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/remote/RemoteDebuggerClient.java - no comments * agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java - no comments * agent/src/share/classes/sun/jvm/hotspot/runtime/VFrame.java - no comments * make/linux/makefiles/sa.make - no comments * make/sa.files - no comments * src/share/vm/runtime/vmStructs.cpp - no comments This is part two of review. Code looks good for me, only minor nits. * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64CFrame.java: 41 missed space around = 51 extra space between pc Done. * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64ThreadContext.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/ppc64/PPC64ThreadContext.java 47, 48 extra space before = Done. * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64Thread.java 34 extra space before id 42 extra space before = , Done. it might be better to create a constant for size of integer. I agree, but this code was just copied from the corresponding AMD64 version and as far as I can see all the other architectures do it the same way so I didn't change it. * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadContext.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadFactory.java -no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64Thread.java 43 missed space before ? it might be be better to reformat this line to usual if and add comments The same here - this is copied code. But I've adjusted it to at least match with the other platforms. *
Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
On 12/11/2014 03:43 PM, Dmitry Samersoff wrote: Jaroslav, You can set SO_LINGER to zero, in this case socket will be closed immediately without waiting in TIME_WAIT But there are no reliable way to predict whether you can take this port or not after you close it. So the only valid solution is to try to connect to a random port and if this attempt fails try another random port. Everything else will cause more or less frequent intermittent failures. Thanks for all the suggestions! http://cr.openjdk.java.net/~jbachorik/8066708/webrev.02 I've enhanced the original patch with the retry logic using different random port if starting the JMX agent on the provided port fails with BindException. I'm keeping there the changes for properly closing the ports opened for the test purposes and also setting the SO_REUSEADDR - anyway, it does not make sense to reuse the ephemeral test ports. I've split the original test_06 test case in order to keep it readable even with the new retry logic - and also to make each test case to test just one scenario. Cheers, -JB- -Dmitry On 2014-12-11 17:06, Jaroslav Bachorik wrote: On 12/09/2014 01:25 PM, Jaroslav Bachorik wrote: On 12/09/2014 01:39 AM, Stuart Marks wrote: On 12/8/14 12:35 PM, Jaroslav Bachorik wrote: Please, review the following test change Issue : https://bugs.openjdk.java.net/browse/JDK-8066708 Webrev: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.00 The test fails very intermittently when RMI registry is trying to bind to a port previously used in the test (via ServerSocket). This seems to be caused by the sockets created via `new ServerSocket(0)` and being in reusable mode. The fix attempts to prevent this by explicitly forbidding the reusable mode. Hi Jaroslav, I happened to see this fly by, and there are (I think) some similar issues going on in the RMI tests. But first I'll note that I don't think setReuseAddress() will have the effect that you want. Typically it's set to true before binding a socket, so that a subsequent bind operation will succeed even if the address/port is already in use. ServerSockets created with new ServerSocket(0) are already bound, and I'm not sure what calling setReuseAddress(false) will do on such sockets. The spec says behavior is undefined, but my bet is that it does nothing. I guess it doesn't hurt to try this out to see if it makes a difference, but I don't have much confidence it will help. The potential similarity to the RMI tests is exemplified by JDK-8049202 (sorry, this bug report isn't open) but briefly this tests the RMI registry as follows: 1. Opens port 1099 using new ServerSocket(1099) [1099 is the default RMI registry port] in order to ensure that 1099 isn't in use by something else already; 2. If this succeeds, it immediately closes the ServerSocket. 3. Then it creates a new RMI registry on port 1099. In principle, this should succeed, yet it fails around 10% of the time on some systems. The error is port already in use. My best theory is that even though the socket has just been closed by a user program, the kernel has to run the socket through some of the socket states such as FIN_WAIT_1, FIN_WAIT_2, or CLOSING before the socket is actually closed and is available for reuse. If a program -- even the same one -- attempts to open a socket on the same port before the socket has reached its final state, it will get an already in use error. If this is true I don't believe that setting SO_REUSEADDR will work if the socket is in one of these final states. (I remember reading this somewhere but I'm not sure where at the moment. I can try to dig it up if there is interest.) I admit this is just a theory and I'm open to alternatives, and I'm also open to hearing about ways to deal with this problem. Could something similar be going on with this JMX test? Hm, this is exactly what happened with this test :( The problem is that the port is reported as available while it is still occupied and RMI registry attempts to start using that port. If setting SO_REUSEADDR does not work then the only solution would be to retry the test case when this exception occurs. Further investigation shows that the problem was rather the client connecting to a socket being shut down. It sounds like setting SO_REUSEADDR to false should prevent this failure. From the ServerSocket javadoc: When a TCP connection is closed the connection may remain in a timeout state for a period of time after the connection is closed (typically known as the TIME_WAIT state or 2MSL wait state). For applications using a well known socket address or port it may not be possible to bind a socket to the required SocketAddress if there is a connection in the timeout state involving the socket address or port. It also turns out that the test does not close the server sockets properly so there might be several sockets being opened or timed out dangling around. I've updated the test so it is setting
Re: 2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
Hi Serguei,
Thank you for making the patches an optional field.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/oops/constantPool.hpp.sdiff.html
198 if (!patched()) {
199 assert(false, a pseudo-string map may exists for patched CP only);
200 return 0;
201 }
Why not
assert(patched(), a pseud-string map must exist for
patched CP only);
Why is this? Is this really a ShouldNotReachHere? should it be false?
215 assert(true, not found a matching entry in pseudo-string map);
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html
Don't you have to move the value of the patched field from the old
constant pool to the new one? I hate to ask but is there merging that
needs to be done? I don't know how to write this test case though. Is
it possible to redefine a class with a constant pool patches with
another that has constant pool patches?
Somehow I thought you'd have to save the value of the cp_patches oops
passed in.
So I was wondering why you can't change this instead:
bool is_pseudo_string_at(int which) {
// A pseudo string is a string that doesn't have a symbol in the cpSlot
-return unresolved_string_at(which) == NULL;
+ return (strncmp(unresolved_string_at(which)-as_utf8(),
CONSTANT_PLACEHOLDER_ , strlen(CONSTANT_PLACEHOLDER)) == 0);
}
And the asserts in the other functions below it.
Coleen
On 12/17/14, 12:26 PM, serguei.spit...@oracle.com wrote:
Please, review the second round fix for:
https://bugs.openjdk.java.net/browse/JDK-8008678
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/
Summary:
This fix implements a footprint saving approach suggested by Coleen.
To be able to reconstitute a class constant pool, an intermediate
pseudo-string map is used.
Now, this field is accounted optionally, only if the 'cp_patches' is
provided in the
ClassFileParser::parseClassFile() before ConstantPool is allocated.
This fix is not elegant, even a little bit ugly, but it is the only
way I see so far.
Unfortunately, this approach did not help much to make some other
fields (eg., 'operands') optional.
The problem is that we have to account optional fields before
parsing, at the CP allocation time.
It is possible to re-allocate the ConstantPool when any
InvokeDynamic bytecode is discovered,
but it looks too complicated.
Testing:
- the unit test from bug report
- nsk.jvmti,testlist, nsk.jdi.testlist, JTREG java/lang/instrument
- vm.mlvm.testlist, vm.quick.testlist,
vm.parallel_class_loading.testlist (in progress)
Thanks,
Serguei
On 11/26/14 11:53 AM, serguei.spit...@oracle.com wrote:
Coleen,
Thank you for looking at this!
I'll check how this can be improved.
It is my concern too.
Thanks,
Serguei
On 11/26/14 9:17 AM, Coleen Phillimore wrote:
Serguei,
I had a quick look at this. I was wondering if we could make the
pseudo_string_map conditional in ConstantPool and not make all
classes pay in footprint for this field? The same thing probably
could be done for operands too. There are flags that you can set to
conditionally add a pointer to base() in this function.
Typical C++ would subclass ConstantPool to add
InvokeDynamicConstantPool fields, but this is not typical C++ so the
trick we use is like the one in ConstMethod. I think it's worth
doing in this case.
Thanks,
Coleen
On 11/26/14, 3:59 AM, serguei.spit...@oracle.com wrote:
Please, review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8008678
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.1/
Summary:
The pseudo-strings are currently not supported in reconstitution
of constant pool.
This is an explanation from John Rose about what the
pseudo-strings are:
We still need live oop constants pre-linked into the constant
pool of bytecodes which
implement some method handles. We use the anonymous class
pseudo-string feature for that.
The relevant code is here:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
These oops are what pseudo-strings are.
The odd name refers to the fact that, even though they are
random oops, they appear in the constant pool
where one would expect (because of class file syntax) to find a
string.
...
If you really wanted to reconstitute a class file for an
anonymous class, and
if that class has oop patching (pseudo-strings), you would need
either to (a) reconstitute the patches array
handed to Unsafe.defineAnonymousClass, or (b) accept whatever
odd strings were there first, as an approximation.
The odd strings are totally insignificant, and are typically
something like CONSTANT_PLACEHOLDER_42
(see
Re: 2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
Hi Coleen,
Thank you for reviewing!
On 12/18/14 11:23 AM, Coleen Phillimore wrote:
Hi Serguei,
Thank you for making the patches an optional field.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/oops/constantPool.hpp.sdiff.html
198 if (!patched()) {
199 assert(false, a pseudo-string map may exists for patched CP only);
200 return 0;
201 }
Why not
assert(patched(), a pseud-string map must exist for
patched CP only);
Wanted it to be more reliable but it looks pointless.
Will make this change.
Why is this? Is this really a ShouldNotReachHere? should it be false?
215 assert(true, not found a matching entry in pseudo-string map);
A typo, must be false.
It is the last minute change.
Thanks for the catch!
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html
Don't you have to move the value of the patched field from the old
constant pool to the new one? I hate to ask but is there merging that
needs to be done? I don't know how to write this test case though.
Is it possible to redefine a class with a constant pool patches with
another that has constant pool patches?
Thank you for asking this question.
If I understand correctly, the patching comes from the compiler side for
anonymous classes only.
I saw it for LambdaForm's only.
I think, the patching can not be changed with a retransformation.
But I'm not sure if it can not be changed with a redefinition.
But if it can - then it would be safe to merge the 'patched' condition,
i.e. make it patched
if either the_class or scratch class is patched.
Somehow I thought you'd have to save the value of the cp_patches oops
passed in.
So I was wondering why you can't change this instead:
bool is_pseudo_string_at(int which) {
// A pseudo string is a string that doesn't have a symbol in the
cpSlot
-return unresolved_string_at(which) == NULL;
+ return (strncmp(unresolved_string_at(which)-as_utf8(),
CONSTANT_PLACEHOLDER_ , strlen(CONSTANT_PLACEHOLDER)) == 0);
}
I was thinking about the same but was not sure if it would work for the
compiler team.
We have to ask John about this (added John and Christian to the cc-list).
This question to John was in my plan! :)
The beauty of the above approach is that there is no need to create an
intermediate
pseudo-string map and most of the code in from this webrev is not needed.
Thanks,
Serguei
And the asserts in the other functions below it.
Coleen
On 12/17/14, 12:26 PM, serguei.spit...@oracle.com wrote:
Please, review the second round fix for:
https://bugs.openjdk.java.net/browse/JDK-8008678
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/
Summary:
This fix implements a footprint saving approach suggested by Coleen.
To be able to reconstitute a class constant pool, an intermediate
pseudo-string map is used.
Now, this field is accounted optionally, only if the 'cp_patches'
is provided in the
ClassFileParser::parseClassFile() before ConstantPool is allocated.
This fix is not elegant, even a little bit ugly, but it is the only
way I see so far.
Unfortunately, this approach did not help much to make some other
fields (eg., 'operands') optional.
The problem is that we have to account optional fields before
parsing, at the CP allocation time.
It is possible to re-allocate the ConstantPool when any
InvokeDynamic bytecode is discovered,
but it looks too complicated.
Testing:
- the unit test from bug report
- nsk.jvmti,testlist, nsk.jdi.testlist, JTREG java/lang/instrument
- vm.mlvm.testlist, vm.quick.testlist,
vm.parallel_class_loading.testlist (in progress)
Thanks,
Serguei
On 11/26/14 11:53 AM, serguei.spit...@oracle.com wrote:
Coleen,
Thank you for looking at this!
I'll check how this can be improved.
It is my concern too.
Thanks,
Serguei
On 11/26/14 9:17 AM, Coleen Phillimore wrote:
Serguei,
I had a quick look at this. I was wondering if we could make the
pseudo_string_map conditional in ConstantPool and not make all
classes pay in footprint for this field? The same thing probably
could be done for operands too. There are flags that you can set
to conditionally add a pointer to base() in this function.
Typical C++ would subclass ConstantPool to add
InvokeDynamicConstantPool fields, but this is not typical C++ so
the trick we use is like the one in ConstMethod. I think it's
worth doing in this case.
Thanks,
Coleen
On 11/26/14, 3:59 AM, serguei.spit...@oracle.com wrote:
Please, review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8008678
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.1/
Summary:
The pseudo-strings are currently not supported in
reconstitution of constant pool.
This is an explanation from John
Re: 2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
On 12/18/14 2:00 PM, serguei.spit...@oracle.com wrote:
Hi Coleen,
Thank you for reviewing!
On 12/18/14 11:23 AM, Coleen Phillimore wrote:
Hi Serguei,
Thank you for making the patches an optional field.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/oops/constantPool.hpp.sdiff.html
198 if (!patched()) {
199 assert(false, a pseudo-string map may exists for patched CP only);
200 return 0;
201 }
Why not
assert(patched(), a pseud-string map must exist for
patched CP only);
Wanted it to be more reliable but it looks pointless.
Will make this change.
Why is this? Is this really a ShouldNotReachHere? should it be false?
215 assert(true, not found a matching entry in pseudo-string map);
A typo, must be false.
It is the last minute change.
Thanks for the catch!
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html
Don't you have to move the value of the patched field from the old
constant pool to the new one? I hate to ask but is there merging
that needs to be done? I don't know how to write this test case
though. Is it possible to redefine a class with a constant pool
patches with another that has constant pool patches?
Thank you for asking this question.
If I understand correctly, the patching comes from the compiler side
for anonymous classes only.
I saw it for LambdaForm's only.
I think, the patching can not be changed with a retransformation.
But I'm not sure if it can not be changed with a redefinition.
But if it can - then it would be safe to merge the 'patched'
condition, i.e. make it patched
if either the_class or scratch class is patched.
Somehow I thought you'd have to save the value of the cp_patches oops
passed in.
Forgot to answer this good question.
If the cp_patches can be changed with a class redefinition (it,
probably, can)
then it needs to be saved and merged too.
Can we separate this cp_patches merge issue into another bug or RFE?
The need still has to be clarified though, I'll check this with John.
Thanks,
Serguei
So I was wondering why you can't change this instead:
bool is_pseudo_string_at(int which) {
// A pseudo string is a string that doesn't have a symbol in the
cpSlot
-return unresolved_string_at(which) == NULL;
+ return (strncmp(unresolved_string_at(which)-as_utf8(),
CONSTANT_PLACEHOLDER_ , strlen(CONSTANT_PLACEHOLDER)) == 0);
}
I was thinking about the same but was not sure if it would work for
the compiler team.
We have to ask John about this (added John and Christian to the cc-list).
This question to John was in my plan! :)
The beauty of the above approach is that there is no need to create an
intermediate
pseudo-string map and most of the code in from this webrev is not needed.
Thanks,
Serguei
And the asserts in the other functions below it.
Coleen
On 12/17/14, 12:26 PM, serguei.spit...@oracle.com wrote:
Please, review the second round fix for:
https://bugs.openjdk.java.net/browse/JDK-8008678
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/
Summary:
This fix implements a footprint saving approach suggested by Coleen.
To be able to reconstitute a class constant pool, an intermediate
pseudo-string map is used.
Now, this field is accounted optionally, only if the 'cp_patches'
is provided in the
ClassFileParser::parseClassFile() before ConstantPool is allocated.
This fix is not elegant, even a little bit ugly, but it is the
only way I see so far.
Unfortunately, this approach did not help much to make some other
fields (eg., 'operands') optional.
The problem is that we have to account optional fields before
parsing, at the CP allocation time.
It is possible to re-allocate the ConstantPool when any
InvokeDynamic bytecode is discovered,
but it looks too complicated.
Testing:
- the unit test from bug report
- nsk.jvmti,testlist, nsk.jdi.testlist, JTREG java/lang/instrument
- vm.mlvm.testlist, vm.quick.testlist,
vm.parallel_class_loading.testlist (in progress)
Thanks,
Serguei
On 11/26/14 11:53 AM, serguei.spit...@oracle.com wrote:
Coleen,
Thank you for looking at this!
I'll check how this can be improved.
It is my concern too.
Thanks,
Serguei
On 11/26/14 9:17 AM, Coleen Phillimore wrote:
Serguei,
I had a quick look at this. I was wondering if we could make the
pseudo_string_map conditional in ConstantPool and not make all
classes pay in footprint for this field? The same thing probably
could be done for operands too. There are flags that you can set
to conditionally add a pointer to base() in this function.
Typical C++ would subclass ConstantPool to add
InvokeDynamicConstantPool fields, but this is not typical C++ so
the trick we use is like the one in ConstMethod. I think it's
worth doing in this case.
Thanks,
Re: 2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
On 12/18/14, 5:23 PM, serguei.spit...@oracle.com wrote:
On 12/18/14 2:00 PM, serguei.spit...@oracle.com wrote:
Hi Coleen,
Thank you for reviewing!
On 12/18/14 11:23 AM, Coleen Phillimore wrote:
Hi Serguei,
Thank you for making the patches an optional field.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/oops/constantPool.hpp.sdiff.html
198 if (!patched()) {
199 assert(false, a pseudo-string map may exists for patched CP only);
200 return 0;
201 }
Why not
assert(patched(), a pseud-string map must exist for
patched CP only);
Wanted it to be more reliable but it looks pointless.
Will make this change.
Why is this? Is this really a ShouldNotReachHere? should it be false?
215 assert(true, not found a matching entry in pseudo-string map);
A typo, must be false.
It is the last minute change.
Thanks for the catch!
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html
Don't you have to move the value of the patched field from the old
constant pool to the new one? I hate to ask but is there merging
that needs to be done? I don't know how to write this test case
though. Is it possible to redefine a class with a constant pool
patches with another that has constant pool patches?
Thank you for asking this question.
If I understand correctly, the patching comes from the compiler side
for anonymous classes only.
I saw it for LambdaForm's only.
I think, the patching can not be changed with a retransformation.
But I'm not sure if it can not be changed with a redefinition.
But if it can - then it would be safe to merge the 'patched'
condition, i.e. make it patched
if either the_class or scratch class is patched.
Somehow I thought you'd have to save the value of the cp_patches
oops passed in.
Forgot to answer this good question.
If the cp_patches can be changed with a class redefinition (it,
probably, can)
then it needs to be saved and merged too.
Can we separate this cp_patches merge issue into another bug or RFE?
The need still has to be clarified though, I'll check this with John.
Yes, it seems good to have a follow up bug for this once the requirement
is understood. It may be a fair bit of work.
Thanks,
Coleen
Thanks,
Serguei
So I was wondering why you can't change this instead:
bool is_pseudo_string_at(int which) {
// A pseudo string is a string that doesn't have a symbol in the
cpSlot
-return unresolved_string_at(which) == NULL;
+ return (strncmp(unresolved_string_at(which)-as_utf8(),
CONSTANT_PLACEHOLDER_ , strlen(CONSTANT_PLACEHOLDER)) == 0);
}
I was thinking about the same but was not sure if it would work for
the compiler team.
We have to ask John about this (added John and Christian to the cc-list).
This question to John was in my plan! :)
The beauty of the above approach is that there is no need to create
an intermediate
pseudo-string map and most of the code in from this webrev is not needed.
Thanks,
Serguei
And the asserts in the other functions below it.
Coleen
On 12/17/14, 12:26 PM, serguei.spit...@oracle.com wrote:
Please, review the second round fix for:
https://bugs.openjdk.java.net/browse/JDK-8008678
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/
Summary:
This fix implements a footprint saving approach suggested by Coleen.
To be able to reconstitute a class constant pool, an intermediate
pseudo-string map is used.
Now, this field is accounted optionally, only if the 'cp_patches'
is provided in the
ClassFileParser::parseClassFile() before ConstantPool is allocated.
This fix is not elegant, even a little bit ugly, but it is the
only way I see so far.
Unfortunately, this approach did not help much to make some other
fields (eg., 'operands') optional.
The problem is that we have to account optional fields before
parsing, at the CP allocation time.
It is possible to re-allocate the ConstantPool when any
InvokeDynamic bytecode is discovered,
but it looks too complicated.
Testing:
- the unit test from bug report
- nsk.jvmti,testlist, nsk.jdi.testlist, JTREG java/lang/instrument
- vm.mlvm.testlist, vm.quick.testlist,
vm.parallel_class_loading.testlist (in progress)
Thanks,
Serguei
On 11/26/14 11:53 AM, serguei.spit...@oracle.com wrote:
Coleen,
Thank you for looking at this!
I'll check how this can be improved.
It is my concern too.
Thanks,
Serguei
On 11/26/14 9:17 AM, Coleen Phillimore wrote:
Serguei,
I had a quick look at this. I was wondering if we could make the
pseudo_string_map conditional in ConstantPool and not make all
classes pay in footprint for this field? The same thing probably
could be done for operands too. There are flags that you can set
to conditionally add a pointer to base() in this function.
Typical C++ would
Re: 2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
On 12/18/14 3:01 PM, Coleen Phillimore wrote:
On 12/18/14, 5:23 PM, serguei.spit...@oracle.com wrote:
On 12/18/14 2:00 PM, serguei.spit...@oracle.com wrote:
Hi Coleen,
Thank you for reviewing!
On 12/18/14 11:23 AM, Coleen Phillimore wrote:
Hi Serguei,
Thank you for making the patches an optional field.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/oops/constantPool.hpp.sdiff.html
198 if (!patched()) {
199 assert(false, a pseudo-string map may exists for patched CP only);
200 return 0;
201 }
Why not
assert(patched(), a pseud-string map must exist
for patched CP only);
Wanted it to be more reliable but it looks pointless.
Will make this change.
Why is this? Is this really a ShouldNotReachHere? should it be
false?
215 assert(true, not found a matching entry in pseudo-string map);
A typo, must be false.
It is the last minute change.
Thanks for the catch!
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html
Don't you have to move the value of the patched field from the old
constant pool to the new one? I hate to ask but is there merging
that needs to be done? I don't know how to write this test case
though. Is it possible to redefine a class with a constant pool
patches with another that has constant pool patches?
Thank you for asking this question.
If I understand correctly, the patching comes from the compiler side
for anonymous classes only.
I saw it for LambdaForm's only.
I think, the patching can not be changed with a retransformation.
But I'm not sure if it can not be changed with a redefinition.
But if it can - then it would be safe to merge the 'patched'
condition, i.e. make it patched
if either the_class or scratch class is patched.
Somehow I thought you'd have to save the value of the cp_patches
oops passed in.
Forgot to answer this good question.
If the cp_patches can be changed with a class redefinition (it,
probably, can)
then it needs to be saved and merged too.
Can we separate this cp_patches merge issue into another bug or RFE?
The need still has to be clarified though, I'll check this with John.
Yes, it seems good to have a follow up bug for this once the
requirement is understood. It may be a fair bit of work.
Ok, thanks!
Serguei
Thanks,
Coleen
Thanks,
Serguei
So I was wondering why you can't change this instead:
bool is_pseudo_string_at(int which) {
// A pseudo string is a string that doesn't have a symbol in
the cpSlot
-return unresolved_string_at(which) == NULL;
+ return (strncmp(unresolved_string_at(which)-as_utf8(),
CONSTANT_PLACEHOLDER_ , strlen(CONSTANT_PLACEHOLDER)) == 0);
}
I was thinking about the same but was not sure if it would work for
the compiler team.
We have to ask John about this (added John and Christian to the
cc-list).
This question to John was in my plan! :)
The beauty of the above approach is that there is no need to create
an intermediate
pseudo-string map and most of the code in from this webrev is not
needed.
Thanks,
Serguei
And the asserts in the other functions below it.
Coleen
On 12/17/14, 12:26 PM, serguei.spit...@oracle.com wrote:
Please, review the second round fix for:
https://bugs.openjdk.java.net/browse/JDK-8008678
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/
Summary:
This fix implements a footprint saving approach suggested by Coleen.
To be able to reconstitute a class constant pool, an
intermediate pseudo-string map is used.
Now, this field is accounted optionally, only if the
'cp_patches' is provided in the
ClassFileParser::parseClassFile() before ConstantPool is allocated.
This fix is not elegant, even a little bit ugly, but it is the
only way I see so far.
Unfortunately, this approach did not help much to make some
other fields (eg., 'operands') optional.
The problem is that we have to account optional fields before
parsing, at the CP allocation time.
It is possible to re-allocate the ConstantPool when any
InvokeDynamic bytecode is discovered,
but it looks too complicated.
Testing:
- the unit test from bug report
- nsk.jvmti,testlist, nsk.jdi.testlist, JTREG java/lang/instrument
- vm.mlvm.testlist, vm.quick.testlist,
vm.parallel_class_loading.testlist (in progress)
Thanks,
Serguei
On 11/26/14 11:53 AM, serguei.spit...@oracle.com wrote:
Coleen,
Thank you for looking at this!
I'll check how this can be improved.
It is my concern too.
Thanks,
Serguei
On 11/26/14 9:17 AM, Coleen Phillimore wrote:
Serguei,
I had a quick look at this. I was wondering if we could make
the pseudo_string_map conditional in ConstantPool and not make
all classes pay in footprint for this field? The same thing
probably could be done for operands too. There are flags that
you can set to
