Re: RFR: JDK-8204994: SA might fail to attach to process with "Windbg Error: WaitForEvent failed"
Hi Alex,
It'd be nice to reduce noise from such intermittent issues and also get
rid of such bugs in the future.
My gut feeling is that we just significantly reduced a probability of
this failure in something
like an order of magnitude but it will still happens once in a month or
a half of year.
This issue should go away completely with 3 or 4 iterations.
The price is not high as the 3rd iteration is going to be rare and the
4th should never happen.
Also, it would not increase complexity.
But no pressure, you are to decide.
I'm just sharing my opinion.
Thanks,
Serguei
On 5/28/20 17:04, Alex Menkov wrote:
Hi Serguei,
With my testing 2nd call always succeeded, but I was able to reproduce
the case only 3 times with my test runs.
I can implement the loop, but number of retries is anyway an arbitrary
value.
--alex
On 05/28/2020 15:44, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
+static HRESULT WaitForEvent(IDebugControl *ptrIDebugControl) {
+ // see JDK-8204994: sometimes WaitForEvent fails with E_ACCESSDENIED,
+ // but succeeded on 2nd call.
+ HRESULT hr = ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT,
INFINITE);
+ if (hr == E_ACCESSDENIED) {
+ // yield current thread use of a processor (short delay).
+ SwitchToThread();
+ hr = ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT, INFINITE);
+ }
+ return hr;
+}
Can the ptrIDebugControl->WaitForEvent fail with E_ACCESSDENIED twice
and succeed on third call?
Would it better to make it a loop with more retry attempts?
Thanks,
Serguei
On 5/27/20 13:54, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8204994
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_WaitForEvent/webrev/
--alex
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering, and
when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache getNestMembers()
because we don;t think it will be called often - it is there to complete
the introspection API of Class rather than being anticipated as used in
a regular programmatic sense. I expect the same is true for
permittedSubclasses(). Do we expect isSealed() to be used often or is it
too just there for completeness? If just for completeness then perhaps a
VM query would be a better compromise on the efficiency front? Otherwise
I can accept the current implementation of isSealed(), and a non-caching
permittedClasses() for this initial implementation of sealed classes. If
efficiency turns out to be a problem for isSealed() then we can revisit
it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected members
for performance and also they may be invalidated when the class is
redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors.
I see ConstantPool::is_klass_or_reference check but can't find where
it validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on this
topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "get" prefix on accessors.
This is the New Style, as opposed to the Classic Style using null
(for "empty" results) and a "get" prefix ("getComponentType") to get
a related type. We may choose to to use the New Style for new
reflection API points, and if so let's not choose N different New
Styles, but one New Style. Personally I like removing "get"; I accept
Optional instead of null; and I also suggest that arrays (if any) be
replaced by (immutable) Lists in the New Style
There are a few existing Class APIs that use the new API style:
Class arrayClass();
Optional describeConstable();
String descriptorString();
This will set up a precedence of the new API style in this class.
Should this new permittedSubclasses method return a List instead of an
array? It's okay with me if you prefer to revert back to the old API
style and follow this up after integration.
4442 * Returns true if this {@linkplain Class} is sealed.
4443 *
* @return returns true if this class is sealed
NIt: {@code true} instead of true - consistent with the style this
class uses
Re: RFR: JDK-8204994: SA might fail to attach to process with "Windbg Error: WaitForEvent failed"
Hi Chris, On 05/28/2020 15:22, Chris Plummer wrote: Hi Alex, Overall looks good to me. I'll be real happy if I never see "WaitForEvent Failed!" again in our CI runs. Just one nit: I hope so. 402 // see JDK-8204994: sometimes WaitForEvent fails with E_ACCESSDENIED, 403 // but succeeded on 2nd call. I think "succeeds" would be better than "succeeded". fixed locally. --alex thanks, Chris On 5/27/20 1:54 PM, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8204994 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_WaitForEvent/webrev/ --alex
Re: RFR: JDK-8204994: SA might fail to attach to process with "Windbg Error: WaitForEvent failed"
Hi Serguei,
With my testing 2nd call always succeeded, but I was able to reproduce
the case only 3 times with my test runs.
I can implement the loop, but number of retries is anyway an arbitrary
value.
--alex
On 05/28/2020 15:44, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
+static HRESULT WaitForEvent(IDebugControl *ptrIDebugControl) {
+ // see JDK-8204994: sometimes WaitForEvent fails with E_ACCESSDENIED,
+ // but succeeded on 2nd call.
+ HRESULT hr = ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT, INFINITE);
+ if (hr == E_ACCESSDENIED) {
+ // yield current thread use of a processor (short delay).
+ SwitchToThread();
+ hr = ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT, INFINITE);
+ }
+ return hr;
+}
Can the ptrIDebugControl->WaitForEvent fail with E_ACCESSDENIED twice
and succeed on third call?
Would it better to make it a loop with more retry attempts?
Thanks,
Serguei
On 5/27/20 13:54, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8204994
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_WaitForEvent/webrev/
--alex
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks for confirming it.
Mandy
On 5/28/20 3:31 PM, Harold Seigel wrote:
Hi Mandy,
The entries in the PermittedSubclasses attribute are constant pool
ConstantClass_info entries. These names get validated by the VM in
this code in ClassFileParser::parse_constant_pool():
for (index = 1; index < length; index++) {
const jbyte tag = cp->tag_at(index).value();
switch (tag) {
case JVM_CONSTANT_UnresolvedClass: {
const Symbol* const class_name = cp->klass_name_at(index);
// check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
break;
}
Thanks, Harold
On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is a valid class descriptor -
can you point me there? (otherwise, maybe define it to be unspecified?)
Re: RFR(XS): 8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD and/or OBSOLETE method(s) found
Hi Coleen, The updated webrev version is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.3/ It has your suggestions addressed: - remove log_is_enabled conditions - move ResourceMark's out of loops Thanks, Serguei On 5/28/20 14:44, serguei.spit...@oracle.com wrote: Hi Coleen, Thank you a lot for reviewing this! On 5/28/20 12:48, coleen.phillim...@oracle.com wrote: Hi Serguei, Sorry for the delay reviewing this again. On 5/18/20 3:30 AM, serguei.spit...@oracle.com wrote: Hi Coleen and potential reviewers, Now, the webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/ has a complete fix for all three failure modes related to the guarantee about OLD and OBSOLETE methods. The root cause are the following optimizations: 1) Optimization based on the flag ik->is_being_redefined(): The problem is that the cpcache method entries of such classes are not being adjusted. It is explained below in the initial RFR summary. The fix is to get rid of this optimization. This seems like a good thing to do even though (actually especially because) I can't re-imagine the logic that went into this optimization. Probably, I've not explained it well enough. The logic was that the class marked as is_being_redefined was considered as being redefined in the current redefinition operation. For classes redefined in current redefinition the cpcache is empty, so there is nothing to adjust. The problem is that classes can be marked as is_being_redefined by doit_prologue of one of the following redefinition operations. In such a case, the VM_RedefineClasses::CheckClass::do_klass fails with this guarantee. It is because the VM_RedefineClasses::CheckClass::do_klass does not have this optimization and does not skip such classes as the VM_RedefineClasses::AdjustAndCleanMetadata::do_class. Without this catch this issue could have unknown consequences in the future execution far away from the root cause. 2) Optimization for array classes based on the flag _has_redefined_Object. The problem is that the vtable method entries are not adjusted for array classes. The array classes have to be adjusted even if the java.lang.Object was redefined by one of previous VM_RedefineClasses operation, not only if it was redefined in the current VM_RedefineClasses operation. The fix is is follow this requirement. This I can't understand. The redefinitions are serialized in safepoints, so why would you need to replace vtable entries for arrays if java.lang.Object isn't redefined in this safepoint? The VM_RedefineClasses::CheckClass::do_klass fails with the same guarantee because of this. It never fails this way with this optimization relaxed. I've already broke my head trying to understand it. It can be because of another bug we don't know yet. 3) Optimization based on the flag _has_null_class_loader which assumes that the Hotspot does not support delegation from the bootstrap class loader to a user-defined class loader. The assumption is that if the current class being redefined has a user-defined class loader as its defining class loader, then all classes loaded by the bootstrap class loader can be skipped for vtable/itable method entries adjustment. The problem is that this assumption is not really correct. There are classes that still need the adjustment. For instance, the class java.util.IdentityHashMap$KeyIterator loaded by the bootstrap class loader has the vtable/itable references to the method: java.util.Iterator.forEachRemaining(java.util.function.Consumer) The class java.util.Iterator is defined by a user-defined class loader. The fix is to get rid of this optimization. Also with this optimization, I'm not sure what the logic was that determined that this was safe, so
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris and David,
Thank you for reviewing this change.
--Best regards,
Daniil
On 5/26/20, 4:33 PM, "Chris Plummer" wrote:
Hi Daniil,
Looks good.
thanks,
Chris
On 5/26/20 10:46 AM, Daniil Titov wrote:
> Hi Chris and David,
>
> Please review a new version of the fix [1] with the changes Chris
suggested.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.02
> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
>
> Thank you,
> Daniil
>
> On 5/22/20, 11:50 AM, "Chris Plummer" wrote:
>
> Hi Daniil,
>
> There is one reference to "jvmwarningmsg" that occurs before it is
> declared while all the rest all come after. It probably would make
sense
> to move its declaration up near the top of the file.
>
> 92 private static void matchListedProcesses(OutputAnalyzer
output) {
> 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX)
> 94 .stderrShouldBeEmptyIgnoreVMWarnings();
> 95 }
>
> We probably use this coding pattern all over the place, but I think
it
> just leads to less readable code. Any reason not to use:
>
> 92 private static void matchListedProcesses(OutputAnalyzer
output) {
> 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX);
> 94 output.stderrShouldBeEmptyIgnoreVMWarnings();
> 95 }
>
> I just don't see the point of the chaining, and don't understand why
all
> these OutputAnalyzer methods return the "this" object in the first
> place. Maybe I'm missing something. I guess maybe there are cases
where
> the OutputAnalyzer object is not already in a local variable, adding
> some value to the chaining, but that's not the case here, and I
think if
> it were the case it would be more readable just to stick the
> OutputAnalyzer object in a local. There one other case of this:
>
>154 private static void matchPerfCounters(OutputAnalyzer
output) {
>155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null,
>156 PERF_COUNTER_REGEX)
>157 .stderrShouldBeEmptyIgnoreVMWarnings();
>158 }
>
> I think you can also add spaces after the commas, and probably make
the
> first stdoutShouldMatchByLine() one line.
>
> thanks,
>
> Chris
>
> On 5/21/20 10:06 PM, Daniil Titov wrote:
> > Please review a webrev [1] that reverts the changes done in
jdk.test.lib.process.OutputAnalyzer in [3].
> >
> > Change [3] modified OutputAnalyzer
stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings
. The current webrev [1] reverts this change and instead makes the tests that
expects an empty stderr from launched j-* tools to filter out '-showversion'
from test options when forwarding test VM option to j*-tools.
> >
> > Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7
tests are in progress.
> >
> > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01
> > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
> > [3] https://bugs.openjdk.java.net/browse/JDK-8242009
> >
> > Thank you,
> > Daniil
> >
> >
>
>
>
>
Re: RFR: JDK-8204994: SA might fail to attach to process with "Windbg Error: WaitForEvent failed"
Hi Alex,
It looks good in general.
+static HRESULT WaitForEvent(IDebugControl *ptrIDebugControl) {
+ // see JDK-8204994: sometimes WaitForEvent fails with E_ACCESSDENIED,
+ // but succeeded on 2nd call.
+ HRESULT hr = ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT, INFINITE);
+ if (hr == E_ACCESSDENIED) {
+// yield current thread use of a processor (short delay).
+SwitchToThread();
+hr = ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT, INFINITE);
+ }
+ return hr;
+}
Can the ptrIDebugControl->WaitForEvent fail
with E_ACCESSDENIED
twice and succeed on third call?
Would it better to make it a loop with more retry attempts?
Thanks,
Serguei
On 5/27/20 13:54, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8204994
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_WaitForEvent/webrev/
--alex
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
The entries in the PermittedSubclasses attribute are constant pool
ConstantClass_info entries. These names get validated by the VM in this
code in ClassFileParser::parse_constant_pool():
for (index = 1; index < length; index++) {
const jbyte tag = cp->tag_at(index).value();
switch (tag) {
case JVM_CONSTANT_UnresolvedClass: {
const Symbol* const class_name = cp->klass_name_at(index);
// check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
break;
}
Thanks, Harold
On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors.
I see ConstantPool::is_klass_or_reference check but can't find where
it validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
Re: RFR: JDK-8204994: SA might fail to attach to process with "Windbg Error: WaitForEvent failed"
Hi Alex, Overall looks good to me. I'll be real happy if I never see "WaitForEvent Failed!" again in our CI runs. Just one nit: 402 // see JDK-8204994: sometimes WaitForEvent fails with E_ACCESSDENIED, 403 // but succeeded on 2nd call. I think "succeeds" would be better than "succeeded". thanks, Chris On 5/27/20 1:54 PM, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8204994 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_WaitForEvent/webrev/ --alex
Re: RFR(XS): 8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD and/or OBSOLETE method(s) found
Hi Coleen, Thank you a lot for reviewing this! On 5/28/20 12:48, coleen.phillim...@oracle.com wrote: Hi Serguei, Sorry for the delay reviewing this again. On 5/18/20 3:30 AM, serguei.spit...@oracle.com wrote: Hi Coleen and potential reviewers, Now, the webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/ has a complete fix for all three failure modes related to the guarantee about OLD and OBSOLETE methods. The root cause are the following optimizations: 1) Optimization based on the flag ik->is_being_redefined(): The problem is that the cpcache method entries of such classes are not being adjusted. It is explained below in the initial RFR summary. The fix is to get rid of this optimization. This seems like a good thing to do even though (actually especially because) I can't re-imagine the logic that went into this optimization. Probably, I've not explained it well enough. The logic was that the class marked as is_being_redefined was considered as being redefined in the current redefinition operation. For classes redefined in current redefinition the cpcache is empty, so there is nothing to adjust. The problem is that classes can be marked as is_being_redefined by doit_prologue of one of the following redefinition operations. In such a case, the VM_RedefineClasses::CheckClass::do_klass fails with this guarantee. It is because the VM_RedefineClasses::CheckClass::do_klass does not have this optimization and does not skip such classes as the VM_RedefineClasses::AdjustAndCleanMetadata::do_class. Without this catch this issue could have unknown consequences in the future execution far away from the root cause. 2) Optimization for array classes based on the flag _has_redefined_Object. The problem is that the vtable method entries are not adjusted for array classes. The array classes have to be adjusted even if the java.lang.Object was redefined by one of previous VM_RedefineClasses operation, not only if it was redefined in the current VM_RedefineClasses operation. The fix is is follow this requirement. This I can't understand. The redefinitions are serialized in safepoints, so why would you need to replace vtable entries for arrays if java.lang.Object isn't redefined in this safepoint? The VM_RedefineClasses::CheckClass::do_klass fails with the same guarantee because of this. It never fails this way with this optimization relaxed. I've already broke my head trying to understand it. It can be because of another bug we don't know yet. 3) Optimization based on the flag _has_null_class_loader which assumes that the Hotspot does not support delegation from the bootstrap class loader to a user-defined class loader. The assumption is that if the current class being redefined has a user-defined class loader as its defining class loader, then all classes loaded by the bootstrap class loader can be skipped for vtable/itable method entries adjustment. The problem is that this assumption is not really correct. There are classes that still need the adjustment. For instance, the class java.util.IdentityHashMap$KeyIterator loaded by the bootstrap class loader has the vtable/itable references to the method: java.util.Iterator.forEachRemaining(java.util.function.Consumer) The class java.util.Iterator is defined by a user-defined class loader. The fix is to get rid of this optimization. Also with this optimization, I'm not sure what the logic was that determined that this was safe, so it's best to remove it. Above makes sense. I don't know the full theory behind this optimization. We only have a comment. All three failure modes are observed with the -Xcomp flag. With all three fixes above in place, the Kitchensink does not fail with this guarantee anymore. http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/src/hotspot/share/oops/cpCache.cpp.udiff.html For logging, the log_trace function will also repeat the 'if' statement and not
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class and
returns a constant empty array.
In fact, ReflectionData caches the derived names and reflected members
for performance and also they may be invalidated when the class is
redefined. It might be okay to add ReflectionData::permittedSubclasses
while `PermittedSubclasses` attribute can't be redefined and getting
this attribute is not performance sensitive. For example, the result
of `getNestMembers` is not cached in ReflectionData. It may be better
not to add it in ReflectionData for modifiable and performance-sensitive
data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors. I
see ConstantPool::is_klass_or_reference check but can't find where it
validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on this
topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "get" prefix on accessors.
This is the New Style, as opposed to the Classic Style using null (for
"empty" results) and a "get" prefix ("getComponentType") to get a
related type. We may choose to to use the New Style for new reflection
API points, and if so let's not choose N different New Styles, but one
New Style. Personally I like removing "get"; I accept Optional instead
of null; and I also suggest that arrays (if any) be replaced by
(immutable) Lists in the New Style
There are a few existing Class APIs that use the new API style:
Class arrayClass();
Optional describeConstable();
String descriptorString();
This will set up a precedence of the new API style in this class. Should
this new permittedSubclasses method return a List instead of an array?
It's okay with me if you prefer to revert back to the old API style and
follow this up after integration.
4442 * Returns true if this {@linkplain Class} is sealed.
4443 *
* @return returns true if this class is sealed
NIt: {@code true} instead of true - consistent with the style this
class uses (in most methods)
test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java
Nit: s/sealed_classes/sealedClasses/
- the test directory/file naming convention use camel case or java
variable name convention.
Thanks
[1] https://github.com/openjdk/valhalla/pull/53#issuecomment-633116043
Re: RFR(XS): 8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD and/or OBSOLETE method(s) found
Hi Serguei,
Sorry for the delay reviewing this again.
On 5/18/20 3:30 AM, serguei.spit...@oracle.com wrote:
Hi Coleen and potential reviewers,
Now, the webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/
has a complete fix for all three failure modes related to the
guarantee about OLD and OBSOLETE methods.
The root cause are the following optimizations:
1) Optimization based on the flag ik->is_being_redefined():
The problem is that the cpcache method entries of such classes are
not being adjusted.
It is explained below in the initial RFR summary.
The fix is to get rid of this optimization.
This seems like a good thing to do even though (actually especially
because) I can't re-imagine the logic that went into this optimization.
2) Optimization for array classes based on the flag
_has_redefined_Object.
The problem is that the vtable method entries are not adjusted for
array classes.
The array classes have to be adjusted even if the java.lang.Object
was redefined
by one of previous VM_RedefineClasses operation, not only if it
was redefined in
the current VM_RedefineClasses operation. The fix is is follow
this requirement.
This I can't understand. The redefinitions are serialized in
safepoints, so why would you need to replace vtable entries for arrays
if java.lang.Object isn't redefined in this safepoint?
3) Optimization based on the flag _has_null_class_loader which
assumes that the Hotspot
does not support delegation from the bootstrap class loader to
auser-defined class
loader.The assumption is that if the current class being redefined
has a user-defined
classloader as its defining class loader, then allclasses loaded
by the bootstrap
class loader can be skipped for vtable/itable method entries
adjustment.
The problem is that this assumption is not really correct. There
are classes that
still need the adjustment. For instance, the class
java.util.IdentityHashMap$KeyIterator
loaded by the bootstrap class loader has the vtable/itable
references to the method:
java.util.Iterator.forEachRemaining(java.util.function.Consumer)
The class java.util.Iterator is defined by a user-defined class
loader.
The fix is to get rid of this optimization.
Also with this optimization, I'm not sure what the logic was that
determined that this was safe, so it's best to remove it. Above makes
sense.
All three failure modes are observed with the -Xcomp flag.
With all three fixes above in place, the Kitchensink does not fail
with this guarantee anymore.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/src/hotspot/share/oops/cpCache.cpp.udiff.html
For logging, the log_trace function will also repeat the 'if' statement
and not allocate the external_name() if logging isn't specified, so you
don't need the 'if' statement above.
+ if (log_is_enabled(Trace, redefine, class, update)) {
+ log_trace(redefine, class, update, constantpool)
+ ("cpc %s entry update: %s", entry_type, new_method->external_name());
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/src/hotspot/share/oops/klassVtable.cpp.udiff.html
Same in two cases here, and you could move the ResourceMark outside the
loop at the top.
Thanks,
Coleen
There is still a JIT compiler relted failure:
https://bugs.openjdk.java.net/browse/JDK-8245128
Kitchensink fails with: assert(destination == (address)-1 ||
destination == entry) failed: b) MT-unsafe modification of inline cache
I also saw this failure but just once:
https://bugs.openjdk.java.net/browse/JDK-8245126
Kitchensink fails with: assert(!method->is_old()) failed: Should
not be installing old methods
Thanks,
Serguei
On 5/15/20 15:14, serguei.spit...@oracle.com wrote:
Hi Coleen,
Thanks a lot for review!
Good suggestion, will use it.
In fact, I've found two more related problems with the same guarantee.
One is with vtable method entries adjustment and another with itable.
This webrev version includes a fix for the vtable related issue:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/
I'm still investigating the itable related issue.
It is interesting that the Kitchensink with Instrumentation modules
enabled is like a Pandora box full of surprises.
New problems are getting discovered after some road blocks are removed.
I've just filed a couple of compiler bugs discovered in this mode of
testing:
https://bugs.openjdk.java.net/browse/JDK-8245126
Kitchensink fails with: assert(!method->is_old()) failed: Should
not be installing old methods
https://bugs.openjdk.java.net/browse/JDK-8245128
Kitchensink fails with: assert(destination == (address)-1 ||
destination == entry) failed: b) MT-unsafe modification of inline cache
Thanks,
Serguei
On 5/15/20 05:12, coleen.phillim...@oracle.com wrote:
Serguei,
Good find!! The fix looks good. I'm sure the optimization wasn't
noticeable and thank you for the
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Sure, you could just have cache_jvmti_state() return a boolean to bail
out immediately for is_old.
dl
On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote:
Hi Dean,
Thank you for looking at this!
Okay. Let me check what cab be done in this direction.
There is no point to cache is_old. The compilation has to bail out if
it is discovered to be true.
Thanks,
Serguei
On 5/28/20 00:59, Dean Long wrote:
This seems OK as long as the memory barriers in the thread state
transitions prevent the C++ compiler from doing something like
reading is_old before reading redefinition_count. I would feel
better if both JVMCI and C1/C2 cached is_old and redefinition_count
at the same time (making sure to be in the _thread_in_vm state), then
bail out based on the cached value of is_old.
dl
On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote:
On 5/25/20 23:39, serguei.spit...@oracle.com wrote:
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
Summary:
The Kitchensink stress test with the Instrumentation module
enabled does
a lot of class retransformations in parallel with all other
stressing.
It provokes the assert at the compiled code installation time:
assert(!method->is_old()) failed: Should not be installing old
methods
The problem is that the CompileBroker::invoke_compiler_on_method
in C2 version
(non-JVMCI tiered compilation) is missing the check that exists
in the JVMCI
part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.
Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI class
redefinition did
not happen while the method was compiled, so all the assumptions
remain correct.
2190 // Cache Jvmti state
2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of
JvmtiExport::redefinition_count() is
cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a class
redefinition
happened which also implies some of methods may become old.
However, the method being compiled can be already old at the point
where the
redefinition counter is cached, so the redefinition counter check
does not help much.
Thanks,
Serguei
Testing:
Ran Kitchensink test with the Instrumentation module enabled in mach5
multiple times for 100 times. Without the fix the test normally fails
a couple of times in 200 runs. It does not fail with the fix anymore.
Will also submit hs tiers1-5.
Thanks,
Serguei
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev:http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov Bug:https://bugs.openjdk.java.net/browse/JDK-8238585 The change avoids making all compiled methods on stack not_entrant when switching a java thread to interpreter only execution for jvmti purposes. It is sufficient to deoptimize the compiled frames on stack. Additionally a handshake is used instead of a vm operation to walk the stack and do the deoptimizations. Testing: JCK and JTREG tests, also in Xcomp mode with fastdebug and release builds on all platforms. Thanks, Richard. See also my question if anyone knows a reason for making the compiled methods not_entrant: http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030339.html
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Dean,
Thank you for looking at this!
Okay. Let me check what cab be done in this direction.
There is no point to cache is_old. The compilation has to bail out
if it is discovered to be true.
Thanks,
Serguei
On 5/28/20 00:59, Dean Long wrote:
This
seems OK as long as the memory barriers in the thread state
transitions prevent the C++ compiler from doing something like
reading is_old before reading redefinition_count. I would feel
better if both JVMCI and C1/C2 cached is_old and
redefinition_count at the same time (making sure to be in the
_thread_in_vm state), then bail out based on the cached value of
is_old.
dl
On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote:
On 5/25/20 23:39, serguei.spit...@oracle.com wrote:
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
Summary:
The Kitchensink stress test with the Instrumentation module
enabled does
a lot of class retransformations in parallel with all other
stressing.
It provokes the assert at the compiled code installation
time:
assert(!method->is_old()) failed: Should not be
installing old methods
The problem is that the
CompileBroker::invoke_compiler_on_method in C2 version
(non-JVMCI tiered compilation) is missing the check that
exists in the JVMCI
part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.
Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI class
redefinition did
not happen while the method was compiled, so all the assumptions
remain correct.
2190 // Cache Jvmti state
2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of
JvmtiExport::redefinition_count() is
cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a class
redefinition
happened which also implies some of methods may become old.
However, the method being compiled can be already old at the
point where the
redefinition counter is cached, so the redefinition counter
check does not help much.
Thanks,
Serguei
Testing:
Ran Kitchensink test with the Instrumentation module enabled in mach5
multiple times for 100 times. Without the fix the test normally fails
a couple of times in 200 runs. It does not fail with the fix anymore.
Will also submit hs tiers1-5.
Thanks,
Serguei
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
This seems OK as long as the memory barriers in the thread state
transitions prevent the C++ compiler from doing something like reading
is_old before reading redefinition_count. I would feel better if both
JVMCI and C1/C2 cached is_old and redefinition_count at the same time
(making sure to be in the _thread_in_vm state), then bail out based on
the cached value of is_old.
dl
On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote:
On 5/25/20 23:39, serguei.spit...@oracle.com wrote:
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
Summary:
The Kitchensink stress test with the Instrumentation module enabled
does
a lot of class retransformations in parallel with all other stressing.
It provokes the assert at the compiled code installation time:
assert(!method->is_old()) failed: Should not be installing old
methods
The problem is that the CompileBroker::invoke_compiler_on_method in
C2 version
(non-JVMCI tiered compilation) is missing the check that exists in
the JVMCI
part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.
Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI class
redefinition did
not happen while the method was compiled, so all the assumptions
remain correct.
2190 // Cache Jvmti state
2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of
JvmtiExport::redefinition_count() is
cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a class
redefinition
happened which also implies some of methods may become old.
However, the method being compiled can be already old at the point
where the
redefinition counter is cached, so the redefinition counter check does
not help much.
Thanks,
Serguei
Testing:
Ran Kitchensink test with the Instrumentation module enabled in mach5
multiple times for 100 times. Without the fix the test normally fails
a couple of times in 200 runs. It does not fail with the fix anymore.
Will also submit hs tiers1-5.
Thanks,
Serguei
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
On 28/05/2020 6:35 am, Harold Seigel wrote:
Hi David,
Please review this updated webrev:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
Changes look good - thanks!
One minor comment:
src/hotspot/share/prims/jvm.cpp
2159 if (length != 0) {
2160 for (int i = 0; i < length; i++) {
The if statement is not needed as the loop will be skipped if length is 0.
On testing:
test/hotspot/jtreg/runtime/modules/SealedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleIntfTest.java
You don't seem to have coverage of the full test matrix. For the
combination of "same module or not" x "same package or not" x "public or
not", there should be 8 test cases: 3 failures and 5 successes. Then you
also have "listed in permits clause" versus "not listed in permits clause".
Then you have all that for classes and interfaces.
---
On the question of whether to use ReflectionData or not that is really
question for the core-libs folk to decide.
Thanks,
David
-
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
It includes the following changes:
* Indentation and simplification changes as suggested below
* If a class is not a valid permitted subclass of its sealed super
then an IncompatibleClassChangeError exception is thrown (as
specified in the JVM Spec) instead of VerifyError.
* Added a check that a non-public subclass must be in the same package
as its sealed super. And added appropriate testing.
* Method Class.permittedSubtypes() was changed.
See also inline comments.
On 5/24/2020 10:28 PM, David Holmes wrote:
Hi Harold,
On 22/05/2020 4:33 am, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
I'll list all relevant commens here rather than interspersing below so
that it is easier to track. Mostly nits below, other than the
is_permitted_subclass check in the VM, and the use of ReflectionData
in java.lang.Class.
--
src/hotspot/share/classfile/classFileParser.cpp
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Nowe there is too little indentation - the subclauses of the
conjunction expression should align[1]
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Fixed. Along with indentation of supports_records().
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3794 } else if (_access_flags.is_final()) {
3795 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3796 } else {
3797 parsed_permitted_subclasses_attribute = true;
3798 }
The indent of the comment at L3793 is wrong, and its placement is
awkward because it relates to the next condition. But we don't have to
use if-else here as any parse error results in immediate return due to
the CHECK macro. So the above can be reformatted as:
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 }
3794 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3795 if (_access_flags.is_final()) {
3796 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3797 }
3798 parsed_permitted_subclasses_attribute = true;
Done.
---
src/hotspot/share/oops/instanceKlass.cpp
The logic in InstanceKlass::has_as_permitted_subclass still does not
implement the rules specified in the JVMS. It only implements a "same
module" check, whereas the JVMS specifies an accessibility requirement
as well.
Done.
730 bool InstanceKlass::is_sealed() const {
731 return _permitted_subclasses != NULL &&
732 _permitted_subclasses !=
Universe::the_empty_short_array() &&
733 _permitted_subclasses->length() > 0;
734 }
Please align subclauses.
Done.
---
src/hotspot/share/prims/jvm.cpp
2159 objArrayHandle result (THREAD, r);
Please remove space after "result".
Done.
As we will always create and return an arry, if you reverse these two
statements:
2156 if (length != 0) {
2157 objArrayOop
