Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
hg: jdk8/tl/jaxp: 8012683: Remove unused, obsolete ObjectFactory classes
Changeset: 37b73984640a Author:joehw Date: 2013-05-20 23:46 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jaxp/rev/37b73984640a 8012683: Remove unused, obsolete ObjectFactory classes Reviewed-by: lancea - src/com/sun/org/apache/xerces/internal/xinclude/ObjectFactory.java - src/com/sun/org/apache/xml/internal/serialize/ObjectFactory.java
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
hg: jdk8/tl/nashorn: 3 new changesets
Changeset: 92164a5742db Author:lagergren Date: 2013-05-20 16:38 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/92164a5742db 8006069: Range analysis first iteration, runtime specializations Reviewed-by: jlaskey, sundar ! src/jdk/nashorn/internal/codegen/CompilationPhase.java ! src/jdk/nashorn/internal/codegen/Compiler.java ! src/jdk/nashorn/internal/codegen/CompilerConstants.java ! src/jdk/nashorn/internal/codegen/MethodEmitter.java + src/jdk/nashorn/internal/codegen/RangeAnalyzer.java + src/jdk/nashorn/internal/codegen/types/Range.java ! src/jdk/nashorn/internal/codegen/types/Type.java ! src/jdk/nashorn/internal/ir/BinaryNode.java ! src/jdk/nashorn/internal/ir/FunctionNode.java ! src/jdk/nashorn/internal/ir/LexicalContext.java ! src/jdk/nashorn/internal/ir/Node.java ! src/jdk/nashorn/internal/ir/Symbol.java ! src/jdk/nashorn/internal/runtime/CompiledFunction.java ! src/jdk/nashorn/internal/runtime/FinalScriptFunctionData.java ! src/jdk/nashorn/internal/runtime/RecompilableScriptFunctionData.java ! src/jdk/nashorn/internal/runtime/ScriptEnvironment.java ! src/jdk/nashorn/internal/runtime/ScriptFunctionData.java ! src/jdk/nashorn/internal/runtime/resources/Options.properties + test/script/basic/ranges_disabled.js + test/script/basic/ranges_disabled.js.EXPECTED + test/script/basic/ranges_enabled.js + test/script/basic/ranges_enabled.js.EXPECTED + test/script/basic/ranges_payload.js Changeset: b558e19d5de5 Author:sundar Date: 2013-05-20 23:04 +0530 URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/b558e19d5de5 8014909: ant test compilation error with JoniTest.java Reviewed-by: jlaskey ! make/build.xml Changeset: 1fd18f40ab52 Author:attila Date: 2013-05-20 21:25 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/1fd18f40ab52 8014797: rename Java.toJavaArray/toJavaScriptArray to Java.to/from, respectively. Reviewed-by: jlaskey, sundar ! docs/JavaScriptingProgrammersGuide.html ! docs/source/javaarray.js ! src/jdk/nashorn/api/scripting/resources/engine.js ! src/jdk/nashorn/internal/objects/NativeJava.java ! src/jdk/nashorn/internal/runtime/resources/Messages.properties ! test/script/basic/NASHORN-556.js ! test/script/basic/javaarrayconversion.js ! test/script/currently-failing/logcoverage.js ! test/script/trusted/NASHORN-638.js ! test/script/trusted/NASHORN-653.js
Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror
Hi Coleen,
Good to see all these oops moving to the mirrors. I think the changes
look good. I let someone else review the SA changes
Some comments below:
On 05/21/2013 12:39 AM, Coleen Phillimore wrote:
Summary: Inject protection_domain, signers, init_lock into
java_lang_Class
Net footprint change is zero except that these fields are in Java heap
rather than metaspace.
There should be some memory saved since we now use compressed oops for
the embedded fields.
This helps a little with InstanceKlass size which is in fixed size
space with UseCompressedKlassPointers. Included serviceability because
there were SA changes to code that I don't know is used.
Future work is to remove the signers field and the unused
SetProtectionDomain function.
open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.hpp.patch
// protection domain
- oop protection_domain() { return _protection_domain; }
- void set_protection_domain(oop pd) {
klass_oop_store(_protection_domain, pd); }
+ oop protection_domain() const;
+ void set_protection_domain(Handle pd);
...
// signers
- objArrayOop signers() const { return _signers; }
- void set_signers(objArrayOop s) { klass_oop_store((oop*)_signers,
s); }
+ objArrayOop signers() const;
+ void set_signers(objArrayOop s);
You don't really need the setters on the InstanceKlass anymore. They are
only used in jvm.cpp where they take a couple of unnecessary
indirections: mirror - IK - mirror-set_protection_domain/set_signers.
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/arrayKlass.cpp.udiff.html
- java_lang_Class::create_mirror(k, CHECK);
+ java_lang_Class::create_mirror(k, Handle(NULL), CHECK);
You use NULL here since typeArrays always return a NULL pd, and
objArrays always returns the pd of the bottom klass?
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.cpp.patch
-void InstanceKlass::oops_do(OopClosure* cl) {
- Klass::oops_do(cl);
-
- cl-do_oop(adr_protection_domain());
- cl-do_oop(adr_signers());
- cl-do_oop(adr_init_lock());
-
- // Don't walk the arrays since they are walked from the ClassLoaderData
objects.
-}
If we could move ArrayKlass::_component_mirror into the j.l.Class, then
_java_mirror would be the only oop in the klasses and we could make
Klass::oops_do non-virtual ...
Another thing. If we could direct-allocate the java mirrors in the old
gen, then we wouldn't have to walk all the klasses during the young GCs.
This would make the GCs a bit less complicated and we could get rid of
these fields in Klass:
// Remembered sets support for the oops in the klasses.
jbyte _modified_oops; // Card Table Equivalent (YC/CMS
support)
jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS support)
thanks,
StefanK
Tested with vm.quick.testlist, JPRT, jtreg java/security tests and
jck8 tests.
Thanks,
Coleen
hg: jdk8/tl/langtools: 8013180: Qualified type reference with annotations in throws list crashes compiler
Changeset: 67cbd6d756f4 Author:jfranck Date: 2013-05-21 12:00 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/67cbd6d756f4 8013180: Qualified type reference with annotations in throws list crashes compiler Reviewed-by: jjg + test/tools/javac/annotations/typeAnnotations/8013180/QualifiedName.java
hg: jdk8/tl/langtools: 7177168: Redundant array copy in UnsharedNameTable
Changeset: 824932ecdbc8 Author:vromero Date: 2013-05-21 11:41 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/824932ecdbc8 7177168: Redundant array copy in UnsharedNameTable Reviewed-by: mcimadamore ! src/share/classes/com/sun/tools/javac/util/UnsharedNameTable.java
Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror
On 05/20/2013 11:39 PM, David Holmes wrote: Hi Coleen, On 21/05/2013 8:39 AM, Coleen Phillimore wrote: Summary: Inject protection_domain, signers, init_lock into java_lang_Class Basic VM changes look fine to me. Thanks! Net footprint change is zero except that these fields are in Java heap rather than metaspace. This helps a little with InstanceKlass size which is in fixed size space with UseCompressedKlassPointers. Included serviceability because there were SA changes to code that I don't know is used. Unsure about the SA changes. Basically you just removed access to the pd and signers, rather than changing it to allow access via the new path. That said I don't know SA so don't know whether it makes sense for SA to access things that are logically part of java.lang.Class; or whether it can access them more directly anyway because they are logically part of java.lang.Class. I did remove these from instanceKlass. It doesn't appear in the SA that it digs into the java mirror so there was nowhere to put these fields. The code that I took out was in places that may not be used and may be bit rotted. I added serviceability team to the review request so someone could comment. I already asked Staffan about this. Thanks, Coleen Thanks, David - Future work is to remove the signers field and the unused SetProtectionDomain function. open webrev at http://cr.openjdk.java.net/~coleenp/8003421/ bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421 Tested with vm.quick.testlist, JPRT, jtreg java/security tests and jck8 tests. Thanks, Coleen
Need help with change
I found during code review comment editing for my change that removes signers and protection domain from the InstanceKlass, that JVMTI code seems to have some sort of call back and knowledge of these fields in instanceKlass. /constant constant id=JVMTI_REFERENCE_SIGNERS num=5 Reference from a class to its signers array. /constant constant id=JVMTI_REFERENCE_PROTECTION_DOMAIN num=6 Reference from a class to its protection domain. If I remove these, will it cause incompatibilities? It's used during jvmtiTagMap.cpp (whatever that's doing). Thanks, Coleen
Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror
Net footprint change is zero except that these fields are in Java heap rather than metaspace. This helps a little with InstanceKlass size which is in fixed size space with UseCompressedKlassPointers. Included serviceability because there were SA changes to code that I don't know is used. Unsure about the SA changes. Basically you just removed access to the pd and signers, rather than changing it to allow access via the new path. That said I don't know SA so don't know whether it makes sense for SA to access things that are logically part of java.lang.Class; or whether it can access them more directly anyway because they are logically part of java.lang.Class. I did remove these from instanceKlass. It doesn't appear in the SA that it digs into the java mirror so there was nowhere to put these fields. The code that I took out was in places that may not be used and may be bit rotted. I added serviceability team to the review request so someone could comment. I already asked Staffan about this. Reading this in more detail, I'm a little worried about the change in hprof output. The change in HeapHprofBinWriter actually breaks the hprof binary format, leading to unparseable files. Granted, this is hprof output when invoked either in SA or with jmap -F so it won't be used by very many people and I can't find any tests that do this. I think we need to find a way to add this information back. /Staffan
Re: Need help with change
When doing heap iteration with JVMTI, the spec requires callbacks from the VM to the agent identifying the signers and protection domain references. This is what tagMap does, see jvmtiTagMap.cpp:2464. As long as it it still possible for JVMTI to find these references (with ik-protection_domain() and ik-signers()), I think it's ok. /Staffan On 21 maj 2013, at 14:52, Coleen Phillimore coleen.phillim...@oracle.com wrote: I found during code review comment editing for my change that removes signers and protection domain from the InstanceKlass, that JVMTI code seems to have some sort of call back and knowledge of these fields in instanceKlass. /constant constant id=JVMTI_REFERENCE_SIGNERS num=5 Reference from a class to its signers array. /constant constant id=JVMTI_REFERENCE_PROTECTION_DOMAIN num=6 Reference from a class to its protection domain. If I remove these, will it cause incompatibilities? It's used during jvmtiTagMap.cpp (whatever that's doing). Thanks, Coleen
hg: jdk8/tl/langtools: 7164114: Two jtreg tests are not run due to no file extension on the test files
Changeset: 08daea43a7f8 Author:vromero Date: 2013-05-21 14:33 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/08daea43a7f8 7164114: Two jtreg tests are not run due to no file extension on the test files Reviewed-by: mcimadamore - test/tools/javac/HiddenAbstractMethod/Test + test/tools/javac/HiddenAbstractMethod/Test.java - test/tools/javac/NonAmbiguousField/Test + test/tools/javac/NonAmbiguousField/Test.java ! test/tools/javac/NonAmbiguousField/two/Child2.java
Re: Need help with change
On 05/21/2013 09:29 AM, Staffan Larsen wrote: When doing heap iteration with JVMTI, the spec requires callbacks from the VM to the agent identifying the signers and protection domain references. This is what tagMap does, see jvmtiTagMap.cpp:2464. As long as it it still possible for JVMTI to find these references (with ik-protection_domain() and ik-signers()), I think it's ok. Okay. Thanks for the quick answer. I was going to rip this out (rats, now I can't). I can get to both protection domain and signers through the mirror. We are working on moving the signers completely to the jdk and not having the jvm know about them at all. Then we can't find the signers through this interface. Should we file a CCC request to change the JVMTI spec then? Thanks, Coleen /Staffan On 21 maj 2013, at 14:52, Coleen Phillimore coleen.phillim...@oracle.com wrote: I found during code review comment editing for my change that removes signers and protection domain from the InstanceKlass, that JVMTI code seems to have some sort of call back and knowledge of these fields in instanceKlass. /constant constant id=JVMTI_REFERENCE_SIGNERS num=5 Reference from a class to its signers array. /constant constant id=JVMTI_REFERENCE_PROTECTION_DOMAIN num=6 Reference from a class to its protection domain. If I remove these, will it cause incompatibilities? It's used during jvmtiTagMap.cpp (whatever that's doing). Thanks, Coleen
Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror
On 05/21/2013 05:11 AM, Stefan Karlsson wrote:
Hi Coleen,
Good to see all these oops moving to the mirrors. I think the changes
look good. I let someone else review the SA changes
Yes, I'm hoping for someone to review the SA changes.
Some comments below:
On 05/21/2013 12:39 AM, Coleen Phillimore wrote:
Summary: Inject protection_domain, signers, init_lock into
java_lang_Class
Net footprint change is zero except that these fields are in Java
heap rather than metaspace.
There should be some memory saved since we now use compressed oops for
the embedded fields.
That's right.
This helps a little with InstanceKlass size which is in fixed size
space with UseCompressedKlassPointers. Included serviceability
because there were SA changes to code that I don't know is used.
Future work is to remove the signers field and the unused
SetProtectionDomain function.
open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.hpp.patch
// protection domain
- oop protection_domain() { return
_protection_domain; }
- void set_protection_domain(oop pd) {
klass_oop_store(_protection_domain, pd); }
+ oop protection_domain() const;
+ void set_protection_domain(Handle pd);
...
// signers
- objArrayOop signers() const { return _signers; }
- void set_signers(objArrayOop s) {
klass_oop_store((oop*)_signers, s); }
+ objArrayOop signers() const;
+ void set_signers(objArrayOop s);
You don't really need the setters on the InstanceKlass anymore. They
are only used in jvm.cpp where they take a couple of unnecessary
indirections: mirror - IK - mirror-set_protection_domain/set_signers.
I left these accessor functions in with a comment that JVMTI spec
defined these fields in InstanceKlass and we have to simulate that they
are still there for compatibility.
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/arrayKlass.cpp.udiff.html
- java_lang_Class::create_mirror(k, CHECK);
+ java_lang_Class::create_mirror(k, Handle(NULL), CHECK);
You use NULL here since typeArrays always return a NULL pd, and
objArrays always returns the pd of the bottom klass?
Yes.
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.cpp.patch
-void InstanceKlass::oops_do(OopClosure* cl) {
- Klass::oops_do(cl);
-
- cl-do_oop(adr_protection_domain());
- cl-do_oop(adr_signers());
- cl-do_oop(adr_init_lock());
-
- // Don't walk the arrays since they are walked from the
ClassLoaderData objects.
-}
If we could move ArrayKlass::_component_mirror into the j.l.Class,
then _java_mirror would be the only oop in the klasses and we could
make Klass::oops_do non-virtual ...
That would add a field to all mirrors though. It's a bit harder but it
would be nice to have smaller mirrors for array klasses vs. instanceKlasses.
Another thing. If we could direct-allocate the java mirrors in the old
gen, then we wouldn't have to walk all the klasses during the young
GCs. This would make the GCs a bit less complicated and we could get
rid of these fields in Klass:
// Remembered sets support for the oops in the klasses.
jbyte _modified_oops; // Card Table Equivalent (YC/CMS
support)
jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS support)
Yes, we want to move in this direction! Not with this change though.
Coleen
thanks,
StefanK
Tested with vm.quick.testlist, JPRT, jtreg java/security tests and
jck8 tests.
Thanks,
Coleen
Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror
On 21 maj 2013, at 16:12, Coleen Phillimore coleen.phillim...@oracle.com
wrote:
On 05/21/2013 05:11 AM, Stefan Karlsson wrote:
Hi Coleen,
Good to see all these oops moving to the mirrors. I think the changes look
good. I let someone else review the SA changes
Yes, I'm hoping for someone to review the SA changes.
Some comments below:
On 05/21/2013 12:39 AM, Coleen Phillimore wrote:
Summary: Inject protection_domain, signers, init_lock into java_lang_Class
Net footprint change is zero except that these fields are in Java heap
rather than metaspace.
There should be some memory saved since we now use compressed oops for the
embedded fields.
That's right.
This helps a little with InstanceKlass size which is in fixed size space
with UseCompressedKlassPointers. Included serviceability because there were
SA changes to code that I don't know is used.
Future work is to remove the signers field and the unused
SetProtectionDomain function.
open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.hpp.patch
// protection domain
- oop protection_domain() { return _protection_domain; }
- void set_protection_domain(oop pd) {
klass_oop_store(_protection_domain, pd); }
+ oop protection_domain() const;
+ void set_protection_domain(Handle pd);
...
// signers
- objArrayOop signers() const { return _signers; }
- void set_signers(objArrayOop s) {
klass_oop_store((oop*)_signers, s); }
+ objArrayOop signers() const;
+ void set_signers(objArrayOop s);
You don't really need the setters on the InstanceKlass anymore. They are
only used in jvm.cpp where they take a couple of unnecessary indirections:
mirror - IK - mirror-set_protection_domain/set_signers.
I left these accessor functions in with a comment that JVMTI spec defined
these fields in InstanceKlass and we have to simulate that they are still
there for compatibility.
Where in the spec does it mention InstanceKlasses?
I still see no reason to keep the setters.
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/arrayKlass.cpp.udiff.html
- java_lang_Class::create_mirror(k, CHECK);
+ java_lang_Class::create_mirror(k, Handle(NULL), CHECK);
You use NULL here since typeArrays always return a NULL pd, and objArrays
always returns the pd of the bottom klass?
Yes.
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.cpp.patch
-void InstanceKlass::oops_do(OopClosure* cl) {
- Klass::oops_do(cl);
-
- cl-do_oop(adr_protection_domain());
- cl-do_oop(adr_signers());
- cl-do_oop(adr_init_lock());
-
- // Don't walk the arrays since they are walked from the ClassLoaderData
objects.
-}
If we could move ArrayKlass::_component_mirror into the j.l.Class, then
_java_mirror would be the only oop in the klasses and we could make
Klass::oops_do non-virtual ...
That would add a field to all mirrors though.
Unless we reuse the pd field, which isn't used for the arrays. But that's a
hack.
It's a bit harder but it would be nice to have smaller mirrors for array
klasses vs. instanceKlasses.
The size of the mirrors are already of variable size, because of the static
fields, so this would probably be a good idea. Something for the Embedded team
to pursue maybe?
Another thing. If we could direct-allocate the java mirrors in the old gen,
then we wouldn't have to walk all the klasses during the young GCs. This
would make the GCs a bit less complicated and we could get rid of these
fields in Klass:
// Remembered sets support for the oops in the klasses.
jbyte _modified_oops; // Card Table Equivalent (YC/CMS support)
jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS support)
Yes, we want to move in this direction! Not with this change though.
Of course. I'm fine with your current changes as they are.
StefanK
Coleen
thanks,
StefanK
Tested with vm.quick.testlist, JPRT, jtreg java/security tests and jck8
tests.
Thanks,
Coleen
Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror
Thanks Stefan, I have 2 small comments.
On 05/21/2013 10:38 AM, Stefan Karlsson wrote:
On 21 maj 2013, at 16:12, Coleen Phillimore coleen.phillim...@oracle.com
wrote:
On 05/21/2013 05:11 AM, Stefan Karlsson wrote:
Hi Coleen,
Good to see all these oops moving to the mirrors. I think the changes look
good. I let someone else review the SA changes
Yes, I'm hoping for someone to review the SA changes.
Some comments below:
On 05/21/2013 12:39 AM, Coleen Phillimore wrote:
Summary: Inject protection_domain, signers, init_lock into java_lang_Class
Net footprint change is zero except that these fields are in Java heap rather
than metaspace.
There should be some memory saved since we now use compressed oops for the
embedded fields.
That's right.
This helps a little with InstanceKlass size which is in fixed size space with
UseCompressedKlassPointers. Included serviceability because there were SA
changes to code that I don't know is used.
Future work is to remove the signers field and the unused SetProtectionDomain
function.
open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.hpp.patch
// protection domain
- oop protection_domain() { return _protection_domain; }
- void set_protection_domain(oop pd) {
klass_oop_store(_protection_domain, pd); }
+ oop protection_domain() const;
+ void set_protection_domain(Handle pd);
...
// signers
- objArrayOop signers() const { return _signers; }
- void set_signers(objArrayOop s) { klass_oop_store((oop*)_signers,
s); }
+ objArrayOop signers() const;
+ void set_signers(objArrayOop s);
You don't really need the setters on the InstanceKlass anymore. They are only used in
jvm.cpp where they take a couple of unnecessary indirections: mirror - IK -
mirror-set_protection_domain/set_signers.
I left these accessor functions in with a comment that JVMTI spec defined these
fields in InstanceKlass and we have to simulate that they are still there for
compatibility.
Where in the spec does it mention InstanceKlasses?
The hprof format and jvmtiTagMap spec assumes they are dumped and the
callbacks are there.
I still see no reason to keep the setters.
Oh, yes, I deleted them. Thanks.
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/arrayKlass.cpp.udiff.html
- java_lang_Class::create_mirror(k, CHECK);
+ java_lang_Class::create_mirror(k, Handle(NULL), CHECK);
You use NULL here since typeArrays always return a NULL pd, and objArrays
always returns the pd of the bottom klass?
Yes.
http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.cpp.patch
-void InstanceKlass::oops_do(OopClosure* cl) {
- Klass::oops_do(cl);
-
- cl-do_oop(adr_protection_domain());
- cl-do_oop(adr_signers());
- cl-do_oop(adr_init_lock());
-
- // Don't walk the arrays since they are walked from the ClassLoaderData
objects.
-}
If we could move ArrayKlass::_component_mirror into the j.l.Class, then
_java_mirror would be the only oop in the klasses and we could make
Klass::oops_do non-virtual ...
That would add a field to all mirrors though.
Unless we reuse the pd field, which isn't used for the arrays. But that's a
hack.
It's a bit harder but it would be nice to have smaller mirrors for array
klasses vs. instanceKlasses.
The size of the mirrors are already of variable size, because of the static
fields, so this would probably be a good idea. Something for the Embedded team
to pursue maybe?
Yes, or a separate change for runtime.
Another thing. If we could direct-allocate the java mirrors in the old gen,
then we wouldn't have to walk all the klasses during the young GCs. This would
make the GCs a bit less complicated and we could get rid of these fields in
Klass:
// Remembered sets support for the oops in the klasses.
jbyte _modified_oops; // Card Table Equivalent (YC/CMS support)
jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS support)
Yes, we want to move in this direction! Not with this change though.
Of course. I'm fine with your current changes as they are.
Thanks! I've made some changes so am retesting. I also changed the
SA code, so I have to send out another review.
Coleen
StefanK
Coleen
thanks,
StefanK
Tested with vm.quick.testlist, JPRT, jtreg java/security tests and jck8 tests.
Thanks,
Coleen
Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror
On 05/21/2013 09:18 AM, Staffan Larsen wrote: Net footprint change is zero except that these fields are in Java heap rather than metaspace. This helps a little with InstanceKlass size which is in fixed size space with UseCompressedKlassPointers. Included serviceability because there were SA changes to code that I don't know is used. Unsure about the SA changes. Basically you just removed access to the pd and signers, rather than changing it to allow access via the new path. That said I don't know SA so don't know whether it makes sense for SA to access things that are logically part of java.lang.Class; or whether it can access them more directly anyway because they are logically part of java.lang.Class. I did remove these from instanceKlass. It doesn't appear in the SA that it digs into the java mirror so there was nowhere to put these fields. The code that I took out was in places that may not be used and may be bit rotted. I added serviceability team to the review request so someone could comment. I already asked Staffan about this. Reading this in more detail, I'm a little worried about the change in hprof output. The change in HeapHprofBinWriter actually breaks the hprof binary format, leading to unparseable files. Granted, this is hprof output when invoked either in SA or with jmap -F so it won't be used by very many people and I can't find any tests that do this. I think we need to find a way to add this information back. Hi, I didn't want to break the hprof format so I added it back to return null for protection domain and signers. There doesn't seem to be a way in SA to read from the mirror but I posted suggested code once this sort of thing is implemented. See: http://cr.openjdk.java.net/~coleenp/8003421_2/agent/src/share/classes/sun/jvm/hotspot/utilities/HeapGXLWriter.java.udiff.html http://cr.openjdk.java.net/~coleenp/8003421_2/agent/src/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java.udiff.html I can file a bug against the SA to get the correct information or decide whether this is something you need to support in this manner. thanks, Coleen /Staffan
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 5/20/2013 10:34 PM, David Holmes wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. I fixed this and will send out a new webrev. joe Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 5/21/2013 2:49 AM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. Okay, it sounds like I should abandon this approach and look into removing the symbol from the symbol files. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. Sounds like returning an error isn't a good idea... joe /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
http://jeremymanson.blogspot.com/2007/05/profiling-with-jvmtijvmpi-sigprof-and.html http://stackoverflow.com/questions/3426537/how-to-properly-write-a-sigprof-handler-that-invokes-asyncgetcalltrace http://hiroshiyamauchi.blogspot.com/2008/12/stabilizing-asyncgetcalltrace.html The AsyncGetCallTrace symbol is a public extern symbol, unfortunately. So there may be third party shared libraries dependent on the extern, and optionally calling it. Removing the symbol will prevent those libraries from linking into the VM, even if they don't call it. Just FYI... -kto On May 21, 2013, at 8:35 AM, JOSEPH PROVINO wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 21 maj 2013, at 18:33, Kelly O'Hair kellyoh...@gmail.com wrote: http://jeremymanson.blogspot.com/2007/05/profiling-with-jvmtijvmpi-sigprof-and.html http://stackoverflow.com/questions/3426537/how-to-properly-write-a-sigprof-handler-that-invokes-asyncgetcalltrace http://hiroshiyamauchi.blogspot.com/2008/12/stabilizing-asyncgetcalltrace.html The AsyncGetCallTrace symbol is a public extern symbol, unfortunately. So there may be third party shared libraries dependent on the extern, and optionally calling it. Removing the symbol will prevent those libraries from linking into the VM, even if they don't call it. I'm willing to break this compatibility for the minimal JVM, given that it's never been supported or official in any way. /Staffan Just FYI... -kto On May 21, 2013, at 8:35 AM, JOSEPH PROVINO wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 21 maj 2013, at 17:35, JOSEPH PROVINO joseph.prov...@oracle.com wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? No, I don't feel strongly either way, but a comment in the code would be nice. Thanks, /Staffan joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 5/21/2013 2:23 PM, Staffan Larsen wrote: On 21 maj 2013, at 17:35, JOSEPH PROVINO joseph.prov...@oracle.com wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, Does anyone know where to find instructions on how to run the collector which would get the error return value? and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? No, I don't feel strongly either way, but a comment in the code would be nice. How much effort should I put into finding out what Sun Studio profiler does when it gets -1? joe Thanks, /Staffan joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 5/21/13 11:23 AM, Staffan Larsen wrote: On 21 maj 2013, at 17:35, JOSEPH PROVINO joseph.prov...@oracle.com wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? No, I don't feel strongly either way, but a comment in the code would be nice. I do not have a strong preference too, but returning an error looks acceptable to me. Thanks, Serguei Thanks, /Staffan joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 5/21/13 11:26 AM, JOSEPH PROVINO wrote: On 5/21/2013 2:23 PM, Staffan Larsen wrote: On 21 maj 2013, at 17:35, JOSEPH PROVINO joseph.prov...@oracle.com wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, Does anyone know where to find instructions on how to run the collector which would get the error return value? and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? No, I don't feel strongly either way, but a comment in the code would be nice. How much effort should I put into finding out what Sun Studio profiler does when it gets -1? Let's ask the Solaris Studio guys directly. I'm adding Oleg to the mailing list. Oleg, Could you, please, share your view on this problem? Thanks, Serguei joe Thanks, /Staffan joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
Though formally not part of the Solaris Studio team any more here is my opinion based on my recollection of how I implemented interaction with the JVM via AsyncGetCallTrace. It's looked up using dlsym. If the symbol is not there Java callstack collection is shut down. I understand in your case even JVMTI is not there so the dlsym call will not be made. From that perspective there is no difference whether the symbol is present and returns an error code or not present at all. -- Oleg On 5/21/2013 12:19 PM, JOSEPH PROVINO wrote: On 5/21/2013 3:16 PM, serguei.spit...@oracle.com wrote: On 5/21/13 11:26 AM, JOSEPH PROVINO wrote: On 5/21/2013 2:23 PM, Staffan Larsen wrote: On 21 maj 2013, at 17:35, JOSEPH PROVINO joseph.prov...@oracle.com wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, Does anyone know where to find instructions on how to run the collector which would get the error return value? and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? No, I don't feel strongly either way, but a comment in the code would be nice. How much effort should I put into finding out what Sun Studio profiler does when it gets -1? Let's ask the Solaris Studio guys directly. I'm adding Oleg to the mailing list. Oleg, Could you, please, share your view on this problem? In particular what will the Sun Studio Profiler collector do if it gets the error trace-num_frames = ticks_no_class_load; // -1 Thanks. joe Thanks, Serguei joe Thanks, /Staffan joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
On 5/21/2013 4:00 PM, Oleg Mazurov wrote: Though formally not part of the Solaris Studio team any more here is my opinion based on my recollection of how I implemented interaction with the JVM via AsyncGetCallTrace. It's looked up using dlsym. If the symbol is not there Java callstack collection is shut down. I understand in your case even JVMTI is not there so the dlsym call will not be made. From that perspective there is no difference whether the symbol is present and returns an error code or not present at all. Oleg, then it sounds like what we have will work. Thanks for the quick reply. joe -- Oleg On 5/21/2013 12:19 PM, JOSEPH PROVINO wrote: On 5/21/2013 3:16 PM, serguei.spit...@oracle.com wrote: On 5/21/13 11:26 AM, JOSEPH PROVINO wrote: On 5/21/2013 2:23 PM, Staffan Larsen wrote: On 21 maj 2013, at 17:35, JOSEPH PROVINO joseph.prov...@oracle.com wrote: On 5/21/2013 3:06 AM, David Holmes wrote: Hi Staffan, On 21/05/2013 4:49 PM, Staffan Larsen wrote: On 21 maj 2013, at 04:34, David Holmes david.hol...@oracle.com wrote: added servicability Hi Joe, As I have previously stated you copied the struct definitions instead of moving them outside the ifdef. Serviceability folk: we are particularly interested in whether the use of ticks_no_class_load is deemed appropriate in this situation. Who will be consuming this value? Since you have opted for the simple fix of having an exported but non-functional AsyncGetCallTrace instead of actually removing the symbol from the symbol files (which is the proposed solution in the bug report), That would be a simpler solution semantically but the only way I can see to do that is to use a text replacement mechanism in the build files - as is done for the dynamic vtable symbols. I find that less appealing than simply exporting an interface that is configured to report an error (which is essentially what all the optional interfaces do under the minimal VM). I would like you to include a comment about this in the source. Right now it's very unclear why there is an exported function that only returns an error. As to the appropriate return value, I don't know. The only caller should be the Sun Studio profiler, Does anyone know where to find instructions on how to run the collector which would get the error return value? and I'm not sure how it will handle this case if ever run. The possible return values aren't very well documented. I guess we need to try and run it to find out. Okay, do either of you feel strongly about how this should be fixed -- return an error or remove the symbol? No, I don't feel strongly either way, but a comment in the code would be nice. How much effort should I put into finding out what Sun Studio profiler does when it gets -1? Let's ask the Solaris Studio guys directly. I'm adding Oleg to the mailing list. Oleg, Could you, please, share your view on this problem? In particular what will the Sun Studio Profiler collector do if it gets the error trace-num_frames = ticks_no_class_load; // -1 Thanks. joe Thanks, Serguei joe Thanks, /Staffan joe Thanks, David /Staffan Thanks, David On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote: The change is to include forte.cpp in the minimal jvm but to conditionalize the code so that only AsyncGetCallTrace() is defined with the minimal jvm. Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/ * JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release https://jbs.oracle.com/bugs/browse/JDK-8013461 Thanks. joe
hg: hsx/hotspot-rt/hotspot: 8014059: JSR292: Failed to reject invalid class cplmhl00201m28n
Changeset: ccdecfece956 Author:bharadwaj Date: 2013-05-21 16:17 -0700 URL: http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/ccdecfece956 8014059: JSR292: Failed to reject invalid class cplmhl00201m28n Summary: Restrict reference of interface methods by invokestatic and invokespecial to classfile version 52 or later. Reviewed-by: kvn, hseigel ! src/share/vm/classfile/classFileParser.cpp
Re: Need help with change
On 21/05/2013 11:56 PM, Coleen Phillimore wrote: On 05/21/2013 09:29 AM, Staffan Larsen wrote: When doing heap iteration with JVMTI, the spec requires callbacks from the VM to the agent identifying the signers and protection domain references. This is what tagMap does, see jvmtiTagMap.cpp:2464. As long as it it still possible for JVMTI to find these references (with ik-protection_domain() and ik-signers()), I think it's ok. Okay. Thanks for the quick answer. I was going to rip this out (rats, now I can't). I can get to both protection domain and signers through the mirror. We are working on moving the signers completely to the jdk and not having the jvm know about them at all. Then we can't find the signers through this interface. Should we file a CCC request to change the JVMTI spec then? You can access any Java object field from the JVM - you just need to add it to java_classes.cpp :) I don't think you can just rip this out of the JVMTI spec. David - Thanks, Coleen /Staffan On 21 maj 2013, at 14:52, Coleen Phillimore coleen.phillim...@oracle.com wrote: I found during code review comment editing for my change that removes signers and protection domain from the InstanceKlass, that JVMTI code seems to have some sort of call back and knowledge of these fields in instanceKlass. /constant constant id=JVMTI_REFERENCE_SIGNERS num=5 Reference from a class to its signers array. /constant constant id=JVMTI_REFERENCE_PROTECTION_DOMAIN num=6 Reference from a class to its protection domain. If I remove these, will it cause incompatibilities? It's used during jvmtiTagMap.cpp (whatever that's doing). Thanks, Coleen
Re: review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release
Hi Kelly, On 22/05/2013 5:27 AM, Kelly O'Hair wrote: I am pretty sure you want to keep the symbol in the library and have it return an error code, rather than remove the symbol entirely. Most tools will not necessarily know what kind of jdk/jre is being used, and getting a runtime linker error means the tool can't get off the ground, unless the tool library is doing a dlsym() to see if the symbol exists or not first, and I kind of doubt they would do that in all cases, but maybe. Kind of depends on what JDK implementations that they might expect their tool to run on. Hard to tell. The tools will have many different features, and maybe the need for this symbol is only part of one feature, and the other features might work fine in a minimal environment. Granted, I'm speaking with very limited knowledge if this detailed discussion, but removing symbols from shared libraries can be a very disruptive thing to tool vendors. Just a warning. FYI the minimalVM doesn't provide any optional VM interfaces. This means that most of the JVM/TI functions are not even present in the binaries (just those needed to ask the question do you have JVM/TI). So we're already well done the path of removing symbols. David - -kto
hg: hsx/hotspot-rt/hotspot: 2 new changesets
Changeset: f54c85acc043 Author:mikael Date: 2013-05-21 09:43 -0700 URL: http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/f54c85acc043 8013726: runtime/memory/ReserveMemory.java fails due to 'assert(bytes % os::vm_allocation_granularity() == 0) failed: reserve block size' Summary: Fix regression test to work on all platforms Reviewed-by: ctornqvi, dholmes ! src/share/vm/prims/whitebox.cpp ! test/runtime/memory/ReserveMemory.java ! test/testlibrary/whitebox/sun/hotspot/WhiteBox.java Changeset: 1a07e086ff28 Author:dholmes Date: 2013-05-21 19:52 -0700 URL: http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/1a07e086ff28 Merge
