On 1/16/15 3:10 PM, serguei.spit...@oracle.com wrote:
On 1/16/15 3:03 PM, Coleen Phillimore wrote:
On 1/16/15, 6:01 PM, serguei.spit...@oracle.com wrote:
John R. suggested to use the CPSlot(Symbol* ptr) to mark pseudo-strings.
I was sort of wondering about this along the same lines. You're
setting the second bit, right? :)
I'm not sure yet.
I'll check if setting the first bit would not create an ambiguity.
Otherwise, will set the second one.
In fact, the suggestion was to look at the PCSlot as an example.
I'm not sure this class is good to extend or reuse for pseudo-strings
as it was added for klass entries only.
Thanks,
Serguei
Coleen
The updated webrev is going to be close to the .3 webrev.
I will send it soon.
Thanks,
Serguei
On 1/16/15 2:53 PM, Coleen Phillimore wrote:
This change looks good to me also.
Thanks,
Coleen
On 1/16/15, 3:07 PM, 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/2015/hotspot/8008678-JVMTI-pseudo.3/
Summary:
Currently, a JVM_CONSTANT_String CP entry having a NULL
reference to Symbol*
indicates that it is a pseudo-string (patched string).
This creates nasty issues for the constant pool reconstitution.
Current suggestion is to avoid having a NULL reference and
retain the original
Symbol* reference for pseudo-strings. The pseudo-string
indication will be
if the Utf8 representation starts from "CONSTANT_PLACEHOLDER_".
This approach makes the fix much simpler.
I need a confirmation from the Compiler team that this won't
break any assumptions or invariants.
Big thanks to Coleen for previous round reviews and good advices!
Testing:
Run:
- java/lang/instrument tests
- new jtreg test (see webrev) that was written by Filipp Zhinkin
Thanks,
Serguei
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.
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 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 java/lang/invoke/InvokerBytecodeGenerator.java)."
Reconstitution of the ConstantPool is needed for both the
JVMTI GetConstantPool() and RetransformClasses().
Finally, it goes to the ConstantPool::copy_cpool_bytes().
The problem is that a pseudo-string is a patched string
that does not have
a reference to the string symbol anymore:
unresolved_string_at(idx) == NULL
The fix is to create and fill in a map from
JVM_CONSTANT_String cp index to the JVM_CONSTANT_Utf8 cp index
to be able to restore this assotiation in the
JvmtiConstantPoolReconstituter.
Testing:
Run:
- java/lang/instrument tests
- new jtreg test (see webrev) that was written by Filipp
Zhinkin
Thanks,
Serguei
