On 23/11/2015 6:03 PM, Jini George wrote:
Thank you David for the review.
The bug report (JDK-7041183) mentions this:
=======Begin bug report snippet ================================
JNIEXPORT jobject JNICALL
Java_sun_management_MemoryPoolImpl_getUsage0
(JNIEnv *env, jobject pool)
{
jobject usage = jmm_interface->GetMemoryPoolUsage(env, pool);
if (usage == NULL) {
// Throw internal error since this implementation expects the
// pool will never become invalid.
JNU_ThrowInternalError(env, "Memory Pool not found");
}
return usage;
}
This function as well as
Java_sun_management_MemoryPoolImpl_getMemoryManagers0,
Java_sun_management_MemoryPoolImpl_getPeakUsage0 should propagate the exception
that may be returned by the jmm call rather than throw InternalError.
=======End bug report snippet ==================================
That assumes there are expected exceptions coming from the lower layer
and was likely based on the fact that these InternalErrors were being
thrown. But as I keep saying either it is a bug if the memory pools are
invalid (in which case InternalError should be thrown) or we have to
identify the circumstances under which they can be invalid and then
update the spec to reflect that and to state what exceptions should be
thrown.
I could not find a scenario where the MemoryPool could not be found, due to
some GC related fixes having gone in in the past. (Hence the closure of the
related https://bugs.openjdk.java.net/browse/JDK-8025089 ). As you said, that
is a scenario which shouldn’t exist. My changes made were to cater to having
the exception thrown from the jmm layer being seen, as mentioned in the bug
report (if in case, the MemoryPool is not found, maybe due to some corruption
or some unforeseen bug which might get introduced later).
Still, having said that, if you and others feel that it is better to retain the
Internal Error, I can close the bug as no change.
If we have not identified legitimate reasons for the pool being invalid
then I would close this as not a bug.
Cheers,
David
-----
Thanks,
Jini.
-----Original Message-----
From: David Holmes
Sent: Monday, November 23, 2015 12:14 PM
To: Jini George; serviceability-dev@openjdk.java.net; Dmitry Samersoff
Subject: Re: RFR(S): JDK-7041183 Improve error handling in Improve
error handling in src/share/vm/services/management.cpp
On 23/11/2015 12:35 PM, Jini George wrote:
Hi David,
Thanks much for the review and for the suggestions offline. I have
addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~dsamersoff/sponsorship/jingeorg/JDK-
704118
3/webrev.02/
src/share/vm/services/management.cpp
424 THROW_MSG_(vmSymbols::java_lang_IllegalStateException(),
425 "Memory Pool should exist",
426 NULL);
why did you use the three arg THROW_MSG_ instead of the two-arg
THROW_MSG?
428 return (MemoryPool*) mpool;
Cast is not needed.
That aside I think we're still miscomunicating on this. Here's what I
last said in our IM chat:
(4:24:02 PM) david.holmes: Basic premise: MemoryPoolMXBeans should
never be invalid.
VM code returns NULL if they are found to be invalid.
JMM library code converts NULL to InternalError because it should not
happen. If it does it is a VM bug.
You found a situation where they were invalid so the cause of that
needs to be determined and either fixed, or else the specifications and
code updated to reflect that there are circumstances where they can be
invalid.
---
Changing the inner layer to throw IllegalStateException does not change
the basic presumption that these pools should never be invalid - in
which case the InternalError seems far more appropriate. Otherwise, as
I
said, you would need to update the specification and the code to allow
for this IllegalStateException - you can't just change the code.
David
-----
(Thanks, Dmitry for hosting). Further comments are inlined below.
-----Original Message-----
On 30/10/2015 9:01 PM, Dmitry Samersoff wrote:
Everybody,
(* On behalf of Jini George <jini.geo...@oracle.com> *)
Please review the fix
http://cr.openjdk.java.net/~dsamersoff/sponsorship/jingeorg/JDK-
704118
3/webrev.01/
If you throw IllegalArgumentException it should be obvious at the
Java
level which argument is illegal and why. I can't tell at the Java
level
how that can come about - these seem to be methods invoked on a
MemoryPoolMXBean - see for example:
https://bugs.openjdk.java.net/browse/JDK-8025089
so the "illegal argument" would seem to be "this" ???
[[Jini]] This has been changed to the more valid
IllegalStateException as you suggested. JDK-8025089 is not reproducible
anymore and has been closed now.
I don't see any changes to the code that would currently throw
Internal
Error ??
[[Jini]] I removed the code throwing the Internal Error from the
upper layer now since this would have masked the exception thrown from
the inner layer.
The asserts for pool!=NULL seem rather pointless as the method has
been
changed to never return null. If you really want the assert add it
to
the end of the method being called, rather than placing at all the
call
sites.
[[Jini]] I removed these asserts now.
Thanks,
Jini.
