Re: Review request (S) 7195557 NPG: Unexpected number of memory pools
Thanks, Mikael! I think this fix looks good also when sent out to the serviceability mailing list :-) I am not a JDK reviewer... Bengt On 2012-09-06 08:54, Mikael Gerdin wrote: Somehow serviceability-dev was dropped from the CC list. I uploaded a new webrev since I thought Bengt had a point about being consistent. We talked about where to integrate the fix during the perm gen removal meeting and decided that we'd like to push this through jdk8/tl/jdk since we're not in a rush for the bug fix, it's already marked as a known failure. Can someone from the jdk8 project please Review this fix? Thanks /Mikael On 2012-09-05 12:38, Mikael Gerdin wrote: Bengt, On 2012-09-05 10:52, Bengt Rutisson wrote: Hi Mikael, I think this looks good. One minor thing. You kind of solved the same problem in two different ways. In CollectionUsageThreshold.java you reduce the expected number and then increase it if you find a perm gen pool. In MemoryMXBean/MemoryTest.java you left the expected value unchanged and only reduce it if we do not find a perm gen pool. I think it would be cleaner to use the same approach for both tests and I think I prefer the way you did it in CollectionUsageThreshold.java since the "normal" case from now on should be that there is no perm gen. Good point, I agree that it looks strange, I've updated the MemoryTest changes to act in a similar way to CollectionUsageThreshold. I've uploaded a new webrev with the updated version: http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1 /Mikael Bengt On 2012-09-05 10:13, Mikael Gerdin wrote: Hi, With the perm gen removal changes the number of memory pools exported by the VM has changed. This causes the tests java/lang/management/MemoryMXBean/MemoryTest.java and java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to fail. My suggested fix is to modify the tests to work with VMs both with and without perm gen memory pools. CR link: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557 Webrev: http://cr.openjdk.java.net/~mgerdin/7195557/webrev I'd also like to request that this fix be allowed to be pushed to hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test failures in the hotspot testing before we integrate perm removal into jdk8. With that I'll need someone to sponsor the push since I'm not a Committer. Thanks /Mikael
Re: Review request (S) 7195557 NPG: Unexpected number of memory pools
Looks good to me, too. Not a Reviewer, either. /Staffan On 6 sep 2012, at 08:58, Bengt Rutisson wrote: > > Thanks, Mikael! I think this fix looks good also when sent out to the > serviceability mailing list :-) > > I am not a JDK reviewer... > > Bengt > > On 2012-09-06 08:54, Mikael Gerdin wrote: >> Somehow serviceability-dev was dropped from the CC list. >> I uploaded a new webrev since I thought Bengt had a point about being >> consistent. >> We talked about where to integrate the fix during the perm gen removal >> meeting and decided that we'd like to push this through jdk8/tl/jdk >> since we're not in a rush for the bug fix, it's already marked as a known >> failure. >> >> Can someone from the jdk8 project please Review this fix? >> Thanks >> /Mikael >> >> On 2012-09-05 12:38, Mikael Gerdin wrote: >>> Bengt, >>> >>> On 2012-09-05 10:52, Bengt Rutisson wrote: Hi Mikael, I think this looks good. One minor thing. You kind of solved the same problem in two different ways. In CollectionUsageThreshold.java you reduce the expected number and then increase it if you find a perm gen pool. In MemoryMXBean/MemoryTest.java you left the expected value unchanged and only reduce it if we do not find a perm gen pool. I think it would be cleaner to use the same approach for both tests and I think I prefer the way you did it in CollectionUsageThreshold.java since the "normal" case from now on should be that there is no perm gen. >>> >>> Good point, I agree that it looks strange, I've updated the MemoryTest >>> changes to act in a similar way to CollectionUsageThreshold. >>> >>> I've uploaded a new webrev with the updated version: >>> http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1 >>> >>> /Mikael >>> Bengt On 2012-09-05 10:13, Mikael Gerdin wrote: > Hi, > > With the perm gen removal changes the number of memory pools exported > by the VM has changed. This causes the tests > java/lang/management/MemoryMXBean/MemoryTest.java and > java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to fail. > > My suggested fix is to modify the tests to work with VMs both with and > without perm gen memory pools. > > CR link: > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557 > Webrev: > http://cr.openjdk.java.net/~mgerdin/7195557/webrev > > I'd also like to request that this fix be allowed to be pushed to > hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test > failures in the hotspot testing before we integrate perm removal into > jdk8. > With that I'll need someone to sponsor the push since I'm not a > Committer. > > Thanks > /Mikael > >>> >> >
Re: Review request (S) 7195557 NPG: Unexpected number of memory pools
On 6/09/2012 5:07 PM, Staffan Larsen wrote: Looks good to me, too. Me too. Not a Reviewer, either. I am :) David - /Staffan On 6 sep 2012, at 08:58, Bengt Rutisson wrote: Thanks, Mikael! I think this fix looks good also when sent out to the serviceability mailing list :-) I am not a JDK reviewer... Bengt On 2012-09-06 08:54, Mikael Gerdin wrote: Somehow serviceability-dev was dropped from the CC list. I uploaded a new webrev since I thought Bengt had a point about being consistent. We talked about where to integrate the fix during the perm gen removal meeting and decided that we'd like to push this through jdk8/tl/jdk since we're not in a rush for the bug fix, it's already marked as a known failure. Can someone from the jdk8 project please Review this fix? Thanks /Mikael On 2012-09-05 12:38, Mikael Gerdin wrote: Bengt, On 2012-09-05 10:52, Bengt Rutisson wrote: Hi Mikael, I think this looks good. One minor thing. You kind of solved the same problem in two different ways. In CollectionUsageThreshold.java you reduce the expected number and then increase it if you find a perm gen pool. In MemoryMXBean/MemoryTest.java you left the expected value unchanged and only reduce it if we do not find a perm gen pool. I think it would be cleaner to use the same approach for both tests and I think I prefer the way you did it in CollectionUsageThreshold.java since the "normal" case from now on should be that there is no perm gen. Good point, I agree that it looks strange, I've updated the MemoryTest changes to act in a similar way to CollectionUsageThreshold. I've uploaded a new webrev with the updated version: http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1 /Mikael Bengt On 2012-09-05 10:13, Mikael Gerdin wrote: Hi, With the perm gen removal changes the number of memory pools exported by the VM has changed. This causes the tests java/lang/management/MemoryMXBean/MemoryTest.java and java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to fail. My suggested fix is to modify the tests to work with VMs both with and without perm gen memory pools. CR link: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557 Webrev: http://cr.openjdk.java.net/~mgerdin/7195557/webrev I'd also like to request that this fix be allowed to be pushed to hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test failures in the hotspot testing before we integrate perm removal into jdk8. With that I'll need someone to sponsor the push since I'm not a Committer. Thanks /Mikael
Re: Review request (S) 7195557 NPG: Unexpected number of memory pools
Thanks for the reviews, I'll try to get someone here in Stockholm to push this to jdk8 for me. /Mikael On 2012-09-06 11:04, David Holmes wrote: On 6/09/2012 5:07 PM, Staffan Larsen wrote: Looks good to me, too. Me too. Not a Reviewer, either. I am :) David - /Staffan On 6 sep 2012, at 08:58, Bengt Rutisson wrote: Thanks, Mikael! I think this fix looks good also when sent out to the serviceability mailing list :-) I am not a JDK reviewer... Bengt On 2012-09-06 08:54, Mikael Gerdin wrote: Somehow serviceability-dev was dropped from the CC list. I uploaded a new webrev since I thought Bengt had a point about being consistent. We talked about where to integrate the fix during the perm gen removal meeting and decided that we'd like to push this through jdk8/tl/jdk since we're not in a rush for the bug fix, it's already marked as a known failure. Can someone from the jdk8 project please Review this fix? Thanks /Mikael On 2012-09-05 12:38, Mikael Gerdin wrote: Bengt, On 2012-09-05 10:52, Bengt Rutisson wrote: Hi Mikael, I think this looks good. One minor thing. You kind of solved the same problem in two different ways. In CollectionUsageThreshold.java you reduce the expected number and then increase it if you find a perm gen pool. In MemoryMXBean/MemoryTest.java you left the expected value unchanged and only reduce it if we do not find a perm gen pool. I think it would be cleaner to use the same approach for both tests and I think I prefer the way you did it in CollectionUsageThreshold.java since the "normal" case from now on should be that there is no perm gen. Good point, I agree that it looks strange, I've updated the MemoryTest changes to act in a similar way to CollectionUsageThreshold. I've uploaded a new webrev with the updated version: http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1 /Mikael Bengt On 2012-09-05 10:13, Mikael Gerdin wrote: Hi, With the perm gen removal changes the number of memory pools exported by the VM has changed. This causes the tests java/lang/management/MemoryMXBean/MemoryTest.java and java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to fail. My suggested fix is to modify the tests to work with VMs both with and without perm gen memory pools. CR link: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557 Webrev: http://cr.openjdk.java.net/~mgerdin/7195557/webrev I'd also like to request that this fix be allowed to be pushed to hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test failures in the hotspot testing before we integrate perm removal into jdk8. With that I'll need someone to sponsor the push since I'm not a Committer. Thanks /Mikael -- Mikael Gerdin Java SE VM SQE Stockholm
hg: jdk8/tl/jdk: 7195557: NPG: Unexpected number of memory pools
Changeset: 076d0dafea5f Author:mgerdin Date: 2012-09-06 14:07 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/076d0dafea5f 7195557: NPG: Unexpected number of memory pools Summary: Update management tests to work with a VM without a permanent generation memory pool Reviewed-by: rbackman, brutisso, sla, dholmes ! test/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java ! test/java/lang/management/MemoryMXBean/MemoryTest.java
Re: Review request (S) 7195557 NPG: Unexpected number of memory pools
(adding serviceability-dev again)
Jon
On 2012-09-06 17:05, Jon Masamitsu wrote:
Mikael,
Does the code in CollectionUsageThreshold.java
happen to work if perm is the last memory pool
in the list and the test
139 if (result.size() == numMemoryPools) {
140 break;
141 }
exits the loop having never seen perm (so not incrementing
numMemoryPools?
Good point. I'll have to look at this tomorrow. Unfortunately this
version of the fix has already been pushed so if we need to fix this
I'll open a new CR.
/Mikael
Jon
On 09/05/12 23:54, Mikael Gerdin wrote:
Somehow serviceability-dev was dropped from the CC list.
I uploaded a new webrev since I thought Bengt had a point about being
consistent.
We talked about where to integrate the fix during the perm gen removal
meeting and decided that we'd like to push this through jdk8/tl/jdk
since we're not in a rush for the bug fix, it's already marked as a
known failure.
Can someone from the jdk8 project please Review this fix?
Thanks
/Mikael
On 2012-09-05 12:38, Mikael Gerdin wrote:
Bengt,
On 2012-09-05 10:52, Bengt Rutisson wrote:
Hi Mikael,
I think this looks good.
One minor thing. You kind of solved the same problem in two different
ways. In CollectionUsageThreshold.java you reduce the expected number
and then increase it if you find a perm gen pool. In
MemoryMXBean/MemoryTest.java you left the expected value unchanged and
only reduce it if we do not find a perm gen pool.
I think it would be cleaner to use the same approach for both tests and
I think I prefer the way you did it in CollectionUsageThreshold.java
since the "normal" case from now on should be that there is no perm
gen.
Good point, I agree that it looks strange, I've updated the MemoryTest
changes to act in a similar way to CollectionUsageThreshold.
I've uploaded a new webrev with the updated version:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1
/Mikael
Bengt
On 2012-09-05 10:13, Mikael Gerdin wrote:
Hi,
With the perm gen removal changes the number of memory pools exported
by the VM has changed. This causes the tests
java/lang/management/MemoryMXBean/MemoryTest.java and
java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to
fail.
My suggested fix is to modify the tests to work with VMs both with and
without perm gen memory pools.
CR link:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557
Webrev:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev
I'd also like to request that this fix be allowed to be pushed to
hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test
failures in the hotspot testing before we integrate perm removal into
jdk8.
With that I'll need someone to sponsor the push since I'm not a
Committer.
Thanks
/Mikael
--
Mikael Gerdin
Java SE VM SQE Stockholm
Fwd: Need reviewers: more predictable binaries
Just FYI... these build changes do touch sources in various teams, please let me know if you have issues with these changes. -kto Begin forwarded message: > From: "Kelly O'Hair" > Subject: Need reviewers: more predictable binaries > Date: September 5, 2012 9:08:53 PM PDT > To: "build-...@openjdk.java.net build-dev" > > > Need a reviewer for this change. > > http://cr.openjdk.java.net/~ohair/openjdk8/jdk8-this-file/webrev/ > > It does change source, but it's effectively a build change. > > The goal here is to try and create more predictable binary files and remove > the possibility that > a full source path (via __FILE__) gets baked into the shared libraries or > executables shipped. > > So two changes: > * sort the .o files during links so they are always provided to the linker > in the same order. > * replace use of __FILE__ to the macro THIS_FILE with just the basename of > the source file being compiled > > The __FILE__ issue is that it has an implementation defined string literal > value, depending on the compiler > and the filename supplied on the compile line. By changing to the new > THIS_FILE macro, the object files > will always have a consistent string literal in them, making it easier to > compare binaries between builds. > Something we have never been able to do in the past. Granted it's just the > basename for the file, but should be enough. > The tricky part is that __FILE__ only gets evaluated when it is used. In > normal C code, it will appear in > macros but it only will get evaluated in the source file being compiled. It > is rare that macros using > __FILE__ will get expanded in include files and I detected this not > happening in the jdk source at all. > > -kto
hg: jdk8/tl/jdk: 7192302: Remove JDBCRowSetImpl dependency on java.beans
Changeset: 8c6895afe204 Author:lancea Date: 2012-09-06 13:16 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8c6895afe204 7192302: Remove JDBCRowSetImpl dependency on java.beans Reviewed-by: alanb, mchung ! src/share/classes/com/sun/rowset/JdbcRowSetImpl.java
Re: [OpenJDK 2D-Dev] Fwd: Need reviewers: more predictable binaries
I feel like I should be able to find the answer to this, but does Objective-C use CPPFLAGS? I can't tell if these changes would apply to .m or .mm files. It certainly appears to be the intent of the change, since a number of .m files in the AWT were changed to use THIS_FILE. -- Scott K. On Sep 6, 2012, at 9:30 AM, Kelly O'Hair wrote: > > Just FYI... > > these build changes do touch sources in various teams, please let me know if > you have issues with these changes. > > -kto > > Begin forwarded message: > >> From: "Kelly O'Hair" >> Subject: Need reviewers: more predictable binaries >> Date: September 5, 2012 9:08:53 PM PDT >> To: "build-...@openjdk.java.net build-dev" >> >> >> Need a reviewer for this change. >> >> http://cr.openjdk.java.net/~ohair/openjdk8/jdk8-this-file/webrev/ >> >> It does change source, but it's effectively a build change. >> >> The goal here is to try and create more predictable binary files and remove >> the possibility that >> a full source path (via __FILE__) gets baked into the shared libraries or >> executables shipped. >> >> So two changes: >> * sort the .o files during links so they are always provided to the linker >> in the same order. >> * replace use of __FILE__ to the macro THIS_FILE with just the basename of >> the source file being compiled >> >> The __FILE__ issue is that it has an implementation defined string literal >> value, depending on the compiler >> and the filename supplied on the compile line. By changing to the new >> THIS_FILE macro, the object files >> will always have a consistent string literal in them, making it easier to >> compare binaries between builds. >> Something we have never been able to do in the past. Granted it's just the >> basename for the file, but should be enough. >> The tricky part is that __FILE__ only gets evaluated when it is used. In >> normal C code, it will appear in >> macros but it only will get evaluated in the source file being compiled. It >> is rare that macros using >> __FILE__ will get expanded in include files and I detected this not >> happening in the jdk source at all. >> >> -kto >
Re: [OpenJDK 2D-Dev] Fwd: Need reviewers: more predictable binaries
My strong suspicion is that the JDK Makefiles only use CFLAGS, not CPPFLAGS for .m files. CPPFLAGS should be used for .mm files (but those should be really rare). Regards, Mike Swingler Apple Inc. On Sep 6, 2012, at 11:35 AM, Scott Kovatch wrote: > I feel like I should be able to find the answer to this, but does Objective-C > use CPPFLAGS? I can't tell if these changes would apply to .m or .mm files. > It certainly appears to be the intent of the change, since a number of .m > files in the AWT were changed to use THIS_FILE. > > -- Scott K. > > On Sep 6, 2012, at 9:30 AM, Kelly O'Hair wrote: > >> >> Just FYI... >> >> these build changes do touch sources in various teams, please let me know if >> you have issues with these changes. >> >> -kto >> >> Begin forwarded message: >> >>> From: "Kelly O'Hair" >>> Subject: Need reviewers: more predictable binaries >>> Date: September 5, 2012 9:08:53 PM PDT >>> To: "build-...@openjdk.java.net build-dev" >>> >>> >>> Need a reviewer for this change. >>> >>> http://cr.openjdk.java.net/~ohair/openjdk8/jdk8-this-file/webrev/ >>> >>> It does change source, but it's effectively a build change. >>> >>> The goal here is to try and create more predictable binary files and remove >>> the possibility that >>> a full source path (via __FILE__) gets baked into the shared libraries or >>> executables shipped. >>> >>> So two changes: >>> * sort the .o files during links so they are always provided to the linker >>> in the same order. >>> * replace use of __FILE__ to the macro THIS_FILE with just the basename of >>> the source file being compiled >>> >>> The __FILE__ issue is that it has an implementation defined string literal >>> value, depending on the compiler >>> and the filename supplied on the compile line. By changing to the new >>> THIS_FILE macro, the object files >>> will always have a consistent string literal in them, making it easier to >>> compare binaries between builds. >>> Something we have never been able to do in the past. Granted it's just the >>> basename for the file, but should be enough. >>> The tricky part is that __FILE__ only gets evaluated when it is used. In >>> normal C code, it will appear in >>> macros but it only will get evaluated in the source file being compiled. It >>> is rare that macros using >>> __FILE__ will get expanded in include files and I detected this not >>> happening in the jdk source at all. >>> >>> -kto >> >
Re: [OpenJDK 2D-Dev] Fwd: Need reviewers: more predictable binaries
Typically, the macros COMPILE.c, COMPILE.CC, and COMPILE.m will include $(CPPFLAGS) which is the preprocessing flags for any compilations using a preprocessor. I was trying to make the minimum changes needed to get this additional -D option on all the compile lines. I am pretty sure this works, but the way I have changed the sources, if I missed any, then the worse case is that you still get __FILE__. It has been suggested that at some point the "#ifndef THIS_FILE" be removed and we expect THIS_FILE to be defined on all compiles, but initially I wasn't willing to be so strict on this. -kto On Sep 6, 2012, at 11:42 AM, Mike Swingler wrote: > My strong suspicion is that the JDK Makefiles only use CFLAGS, not CPPFLAGS > for .m files. CPPFLAGS should be used for .mm files (but those should be > really rare). > > Regards, > Mike Swingler > Apple Inc. > > On Sep 6, 2012, at 11:35 AM, Scott Kovatch wrote: > >> I feel like I should be able to find the answer to this, but does >> Objective-C use CPPFLAGS? I can't tell if these changes would apply to .m or >> .mm files. It certainly appears to be the intent of the change, since a >> number of .m files in the AWT were changed to use THIS_FILE. >> >> -- Scott K. >> >> On Sep 6, 2012, at 9:30 AM, Kelly O'Hair wrote: >> >>> >>> Just FYI... >>> >>> these build changes do touch sources in various teams, please let me know >>> if you have issues with these changes. >>> >>> -kto >>> >>> Begin forwarded message: >>> From: "Kelly O'Hair" Subject: Need reviewers: more predictable binaries Date: September 5, 2012 9:08:53 PM PDT To: "build-...@openjdk.java.net build-dev" Need a reviewer for this change. http://cr.openjdk.java.net/~ohair/openjdk8/jdk8-this-file/webrev/ It does change source, but it's effectively a build change. The goal here is to try and create more predictable binary files and remove the possibility that a full source path (via __FILE__) gets baked into the shared libraries or executables shipped. So two changes: * sort the .o files during links so they are always provided to the linker in the same order. * replace use of __FILE__ to the macro THIS_FILE with just the basename of the source file being compiled The __FILE__ issue is that it has an implementation defined string literal value, depending on the compiler and the filename supplied on the compile line. By changing to the new THIS_FILE macro, the object files will always have a consistent string literal in them, making it easier to compare binaries between builds. Something we have never been able to do in the past. Granted it's just the basename for the file, but should be enough. The tricky part is that __FILE__ only gets evaluated when it is used. In normal C code, it will appear in macros but it only will get evaluated in the source file being compiled. It is rare that macros using __FILE__ will get expanded in include files and I detected this not happening in the jdk source at all. -kto >>> >> >
Re: Review request (S) 7195557 NPG: Unexpected number of memory pools
Mikael,
On 9/6/2012 8:40 AM, Mikael Gerdin wrote:
On 2012-09-06 17:05, Jon Masamitsu wrote:
Mikael,
Does the code in CollectionUsageThreshold.java
happen to work if perm is the last memory pool
in the list and the test
139 if (result.size() == numMemoryPools) {
140 break;
141 }
exits the loop having never seen perm (so not incrementing
numMemoryPools?
Good point. I'll have to look at this tomorrow. Unfortunately this
version of the fix has already been pushed so if we need to fix this
I'll open a new CR.
FYI - the following check was added as part of the fix for:
4959889 Spec change: Revise low memory detection mechanism
if (result.size() != EXPECTED_NUM_POOLS) {
throw new RuntimeException("Unexpected number of selected pools");
}
I believe L139-141 is a test bug that should have been removed
when the above check was added. The next time when you modify
this test, it'd be good to consider modernizing this test to
use for-each and generics. Many of the j.l.m. tests were written
during the development of JDK 5 language support.
Hope this helps.
Mandy
hg: jdk8/tl/jdk: 7196677: diff compares same file to itself in PaddingTest regression test.
Changeset: 833f4630f3a1 Author:weijun Date: 2012-09-07 10:24 +0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/833f4630f3a1 7196677: diff compares same file to itself in PaddingTest regression test. Reviewed-by: xuelei ! test/com/sun/crypto/provider/Cipher/DES/PaddingTest.java
