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