SUGGESTED FIX
The previous suggested fix is just a band-aid.
This fix is the real bug, and should have been part of the putback for 6471009.
diff -r f0a77608b96a src/share/vm/code/dependencies.cpp
--- a/src/share/vm/code/dependencies.cpp Thu Nov 08 16:47:30 2007 -0800
+++ b/src/share/vm/code/dependencies.cpp Tue Nov 27 15:55:00 2007 -0800
@@ -881,6 +881,14 @@ klassOop ClassHierarchyWalker::find_witn
assert(must_be_in_vm(), "raw oops here");
// Must not move the class hierarchy during this check:
assert_locked_or_safepoint(Compile_lock);
+
+ int nof_impls = instanceKlass::cast(context_type)->nof_implementors();
+ if (nof_impls > 1) {
+ // Avoid this case: *I.m > { A.m, C }; B.m > C
+ // %%% Until this is fixed more systematically, bail out.
+ // See corresponding comment in find_witness_anywhere.
+ return context_type;
+ }
assert(!is_participant(new_type), "only old classes are participants");
if (participants_hide_witnesses) {
diff -r f0a77608b96a src/share/vm/code/nmethod.cpp
--- a/src/share/vm/code/nmethod.cpp Thu Nov 08 16:47:30 2007 -0800
+++ b/src/share/vm/code/nmethod.cpp Tue Nov 27 15:55:00 2007 -0800
@@ -1971,7 +1971,7 @@ void nmethod::print_dependencies() {
if (ctxk != NULL) {
Klass* k = Klass::cast(ctxk);
if (k->oop_is_instance() && ((instanceKlass*)k)->is_dependent_nmethod(this)) {
- tty->print(" [nmethod<=klass]%s", k->external_name());
+ tty->print_cr(" [nmethod<=klass]%s", k->external_name());
}
}
deps.log_dependency(); // put it into the xml log also
Sample test run:
-XX:+PrintCompilation -XX:+TraceDependencies DependencyBug
VM option '+PrintCompilation'
VM option '+TraceDependencies'
1 java.lang.String::hashCode (60 bytes)
2 java.lang.String::charAt (33 bytes)
3 java.lang.String::indexOf (151 bytes)
4 DependencyBug::callMethod (7 bytes)
Warning: TraceDependencies results may be inflated by VerifyDependencies
**** Initiating class loading of Impl2 ****
5 java.lang.String::lastIndexOf (156 bytes)
6 java.lang.String::indexOf (166 bytes)
Failed dependency of type unique_concrete_method
context = *DependencyBug$Interface
method = {method} 'method' '()V' in 'DependencyBug$Impl1'
witness = *DependencyBug$Interface
code: 4 nmethod DependencyBug::callMethod (7 bytes)
Marked for deoptimization
context = DependencyBug$Interface
dependee = DependencyBug$Impl2
context supers = 2, interfaces = 1
Compiled (c1) 4 nmethod DependencyBug::callMethod (7 bytes)
total in heap [0xfbef1cc8,0xfbef1ec4] = 508
relocation [0xfbef1d7c,0xfbef1da4] = 40
main code [0xfbef1db0,0xfbef1e20] = 112
stub code [0xfbef1e20,0xfbef1e48] = 40
scopes data [0xfbef1e48,0xfbef1e60] = 24
scopes pcs [0xfbef1e60,0xfbef1ea8] = 72
dependencies [0xfbef1ea8,0xfbef1eac] = 4
nul chk table [0xfbef1eac,0xfbef1eb8] = 12
oops [0xfbef1eb8,0xfbef1ec4] = 12
Dependencies:
Dependency of type unique_concrete_method
context = *DependencyBug$Interface
method = {method} 'method' '()V' in 'DependencyBug$Impl1'
[nmethod<=klass]DependencyBug$Interface
4 made not entrant DependencyBug::callMethod (7 bytes)
#
|
|
|
EVALUATION
My previous evaluation isn't quite right. The original dependency issued by C1 is correct, and used to work, and was broken by the change that created "spot checks" for dependencies.
The bail-out on nof_impls > 1 in find_witness_anywhere needs to be duplicated in the optimized query find_witness at. See suggested fix.
|
|
|
EVALUATION
The test case is described in the comments in dependencies.cpp, around line 961:
// Avoid this case: *I.m > { A.m, C }; B.m > C
// Here, I.m has 2 concrete implementations, but m appears unique
// as A.m, because the search misses B.m when checking C.
// The inherited method B.m was getting missed by the walker
// when interface 'I' was the starting point.
// %%% Until this is fixed more systematically, bail out.
// (Old CHA had the same limitation.)
return context_type;
The question is, if the dependency module (apparently) knows about this case, why isn't it getting asked about it?
I think the code in c1_GraphBuilder.cpp (near 1614) needs an extra dependency to detect when nof_implementors changes.
ciInstanceKlass* singleton = NULL;
if (target->holder()->nof_implementors() == 1) {
singleton = target->holder()->implementor(0);
}
if (singleton) {
cha_monomorphic_target = target->find_monomorphic_target(calling_klass, target->holder(), singleton);
if (cha_monomorphic_target != NULL) {
// If CHA is able to bind this invoke then update the class
// to match that class, otherwise klass will refer to the
// interface.
klass = cha_monomorphic_target->holder();
+ dependency_recorder()->assert_unique_concrete_subtype(actual_recv, singleton);
actual_recv = target->holder();
Since the C1 code updates actual_recv to a unique implementor, it is C1's responsibility to issue a dependency on that uniqueness. The recent changes in b20 made the dependency checking less sloppy. As a side effect, this missing dependency is now exposed.
|
|
|
SUGGESTED FIX
--------
hg diff
diff -r f0a77608b96a src/share/vm/c1/c1_GraphBuilder.cpp
--- a/src/share/vm/c1/c1_GraphBuilder.cpp Thu Nov 08 16:47:30 2007 -0800
+++ b/src/share/vm/c1/c1_GraphBuilder.cpp Wed Nov 21 12:07:25 2007 -0800
@@ -1610,7 +1610,7 @@ void GraphBuilder::invoke(Bytecodes::Cod
if (target->holder()->nof_implementors() == 1) {
singleton = target->holder()->implementor(0);
}
- if (singleton) {
+ if (singleton && !singleton->is_abstract()) {
cha_monomorphic_target = target->find_monomorphic_target(calling_klass, target->holder(), singleton);
if (cha_monomorphic_target != NULL) {
// If CHA is able to bind this invoke then update the class
@@ -1618,6 +1618,7 @@ void GraphBuilder::invoke(Bytecodes::Cod
// interface.
klass = cha_monomorphic_target->holder();
actual_recv = target->holder();
+ dependency_recorder()->assert_abstract_with_unique_concrete_subtype(actual_recv, singleton);
// insert a check it's really the expected class.
CheckCast* c = new CheckCast(klass, receiver, NULL);
--------
Sample test run:
--------
java -XX:+PrintCompilation -XX:+TraceDependencies DependencyBug
VM option '+PrintCompilation'
VM option '+TraceDependencies'
1 java.lang.String::hashCode (60 bytes)
2 java.lang.String::charAt (33 bytes)
3 java.lang.String::indexOf (151 bytes)
**** Initiating class loading of Impl2 ****
4 DependencyBug::callMethod (7 bytes)
Warning: TraceDependencies results may be inflated by VerifyDependencies
5 java.lang.String::lastIndexOf (156 bytes)
Failed dependency of type abstract_with_unique_concrete_subtype
context = *DependencyBug$Interface
class = DependencyBug$Impl1
witness = DependencyBug$Impl2
code: 4 nmethod DependencyBug::callMethod (7 bytes)
Marked for deoptimization
context = DependencyBug$Interface
dependee = DependencyBug$Impl2
context supers = 2, interfaces = 1
Compiled (c1) 4 nmethod DependencyBug::callMethod (7 bytes)
total in heap [0xfbef1cc8,0xfbef1ecc] = 516
relocation [0xfbef1d7c,0xfbef1da4] = 40
main code [0xfbef1db0,0xfbef1e20] = 112
stub code [0xfbef1e20,0xfbef1e48] = 40
scopes data [0xfbef1e48,0xfbef1e60] = 24
scopes pcs [0xfbef1e60,0xfbef1ea8] = 72
dependencies [0xfbef1ea8,0xfbef1eb0] = 8
nul chk table [0xfbef1eb0,0xfbef1ebc] = 12
oops [0xfbef1ebc,0xfbef1ecc] = 16
Dependencies:
Dependency of type abstract_with_unique_concrete_subtype
context = *DependencyBug$Interface
class = DependencyBug$Impl1
[nmethod<=klass]DependencyBug$InterfaceDependency of type unique_concrete_method
context = *DependencyBug$Interface
method = {method} 'method' '()V' in 'DependencyBug$Impl1'
[nmethod<=klass]DependencyBug$Interface 4 made not entrant DependencyBug::callMethod (7 bytes)
#
--------
|
|
|
EVALUATION
The provided test case fails on solaris-sparc and on windows-x86,
but only with the client compiler.
This looks very much like a hotspot compiler bug, so I am redispatching
to hotspot/compiler1 (although investigation to find the b20 change
that caused this should continue in any case)
|
|
|
EVALUATION
It appears the changes for 6471009 cause us to miss a dependence.
% /java/re/jdk/1.7.0/promoted/all/b20/binaries/solaris-sparc/fastdebug/bin/java -client -Xbatch VectorIntegerTest Should have been marked for deoptimization:
dependee = java.util.AbstractList$ListItr
context supers = 2, interfaces = 2
Compiled (c1) 13 nmethod java.util.AbstractList::equals (117 bytes)
total in heap [0xfb10a608,0xfb10b28c] = 3204
relocation [0xfb10a6bc,0xfb10a798] = 220
main code [0xfb10a7a0,0xfb10ace0] = 1344
stub code [0xfb10ace0,0xfb10ad98] = 184
scopes data [0xfb10ad98,0xfb10af60] = 456
scopes pcs [0xfb10af60,0xfb10b218] = 696
dependencies [0xfb10b218,0xfb10b220] = 8
nul chk table [0xfb10b220,0xfb10b254] = 52
oops [0xfb10b254,0xfb10b28c] = 56
Dependencies:
Dependency of type unique_concrete_method
context = *java.util.ListIterator
method = {method} 'hasNext' '()Z' in 'java/util/Vector$Itr'
[nmethod<=klass]java.util.ListIteratorDependency of type unique_concrete_method
context = *java.util.ListIterator
method = {method} 'next' '()Ljava/lang/Object;' in 'java/util/Vector$Itr'
[nmethod<=klass]java.util.ListIteratorException in thread "main" java.lang.IncompatibleClassChangeError
at java.util.AbstractList.equals(AbstractList.java:522)
at java.util.Vector.equals(Vector.java:953)
at VectorIntegerTest.main(VectorIntegerTest.java:29)
|
|
|
EVALUATION
This is deeply mysterious. On one machine the supplied test works perfectly,
while on another, using the *exact*same* binaries, it fails:
(mb29450@suttles) ~/src/toy/6610906 $ time jver -v /net/sqindia.india/export/disk09/jdk/7/b20/binaries/solsparc jr VectorIntegerTest
Using JDK /net/sqindia.india/export/disk09/jdk/7/b20/binaries/solsparc
==> javac -Xlint:all VectorIntegerTest.java
==> java -esa -ea VectorIntegerTest
(mb29450@seetharama) ~/src/toy/6610906 $ jver -v /net/sqindia.india/export/disk09/jdk/7/b20/binaries/solsparc java VectorIntegerTest
Using JDK /net/sqindia.india/export/disk09/jdk/7/b20/binaries/solsparc
Exception in thread "main" java.lang.IncompatibleClassChangeError
at java.util.AbstractList.equals(AbstractList.java:522)
at java.util.Vector.equals(Vector.java:953)
at VectorIntegerTest.VectorIntegerTestTest01(VectorIntegerTest.java:62)
at VectorIntegerTest.main(VectorIntegerTest.java:8)
Seetharam, could you reduce the test case to the minimum required to reproduce this?
It's starting to look like a hotspot or ClassLoader bug.
|
|
|
|
Java