Changing lots of files - mainly GC code (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Tue Jan 14 12:40:54 PST 2014

On 01/14/2014 02:29 PM, Krystal Mok wrote:

Hi Coleen,

Thanks for the review. I'm all for getting things right, and I agree there's more comments that needs some love. I'm okay with leaving the changes in method.hpp out for now, and correct them later.

The ones you and Jesper have so far are fine. You can leave them in.
I would like you to leave them in. I was only pointing out that there are more changes needed that could be done by themselves later.

Coleen

Thanks, Kris

On Wed, Jan 15, 2014 at 2:46 AM, Coleen Phillimore <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>> wrote: These changes you have look good for Method except a lot of the comment is out of date. The block comment about the layout of fields in Method is redundant with the actual declaration and wrong. It's a nice visual but I don't see the point of keeping redundant information like that. Also, it the comment describes some of the fields that were moved to ConstMethod years ago. I don't think you want to try to correct these things with this typo change, but we should (I could) file an RFE to correct them later. thanks, Coleen On 01/14/2014 12:53 PM, Krystal Mok wrote: Hi Jesper,

That looks fine to me. Thank you! It's still better if someone from the runtime team could verify the change in oops/method.hpp, and someone from the compiler team could verify the change in ci/ciField.hpp|cpp Thanks, Kris

On Tue, Jan 14, 2014 at 9:38 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com_ _<mailto:jesper.wilhelmsson at oracle.com>> wrote: Sure, no problem. Webrev is updated, please verify. I took the liberty of changing the layout of the comment in ciField.cpp. Since there was only one case left it didn't feel motivated to have a list of cases. /Jesper Krystal Mok skrev 13/1/14 7:36 PM: Hi Jesper, Thanks! I took a look and the change was fine. I do have a couple of other comment fixes that I'd like to hitchhike: 1. oops/method.hpp The comments say ConstMethod* and MethodData* are oops, which they aren't anymore. I'm not sure if the wording for "putting oops and methodsize first for better gc cache locality" still matters. It probably doesn't matter anymore, since GC doesn't have to mark through these fields with the PermGen removal. 2. ci/ciField.hpp, ci/ciField.cpp The comments say in order to consider a field as a constant, it cannot hold an non-perm-space oop. I believe this limitation has been removed. Could someone from the compiler team verify if that's true? The diff is at the end of this mail. Thanks, Kris diff -r 9d39e8a8ff61 src/share/vm/ci/ciField.cpp --- a/src/share/vm/ci/ciField.cppFri Dec 27 07:51:07 2013 -0800 +++ b/src/share/vm/ci/ciField.cppMon Jan 13 10:31:17 2014 -0800 @@ -202,13 +202,9 @@ } // This field just may be constant. The only cases where it will - // not be constant are: + // not be constant is: // - // 1. The field holds a non-perm-space oop. The field is, strictly - // speaking, constant but we cannot embed non-perm-space oops into - // generated code. For the time being we need to consider the - // field to be not constant. - // 2. The field is a special static&final field whose value + // 1. The field is a special static&final field whose value // may change. The three examples are java.lang.System.in <http://java.lang.System.in> <http://java.lang.System.in>, // java.lang.System.out, and java.lang.System.err. diff -r 9d39e8a8ff61 src/share/vm/ci/ciField.hpp --- a/src/share/vm/ci/ciField.hppFri Dec 27 07:51:07 2013 -0800 +++ b/src/share/vm/ci/ciField.hppMon Jan 13 10:31:17 2014 -0800 @@ -130,9 +130,7 @@ // 1. The field is both static and final // 2. The canonical holder of the field has undergone // static initialization. - // 3. If the field is an object or array, then the oop - // in question is allocated in perm space. - // 4. The field is not one of the special static/final + // 3. The field is not one of the special static/final // non-constant fields. These are java.lang.System.in <http://java.lang.System.in> <http://java.lang.System.in> // and java.lang.System.out. Abomination. // diff -r 9d39e8a8ff61 src/share/vm/oops/method.hpp --- a/src/share/vm/oops/method.hppFri Dec 27 07:51:07 2013 -0800 +++ b/src/share/vm/oops/method.hppMon Jan 13 10:31:17 2014 -0800 @@ -38,13 +38,11 @@ #include "utilities/accessFlags.hpp" #include "utilities/growableArray.hpp" -// A Method* represents a Java method. +// A Method represents a Java method. // // Memory layout (each line represents a word). Note that most applications load thousands of methods, // so keeping the size of this structure small has a big impact on footprint. // -// We put all oops and methodsize first for better gc cache locality. -// // The actual bytecodes are inlined after the end of the Method struct. // // There are bits in the accessflags telling whether inlined tables are present. @@ -64,17 +62,17 @@ // | header | // | klass | // |------------------------------------------------------| -// | ConstMethod* (oop) | +// | ConstMethod* (metadata) | // |------------------------------------------------------| -// | methodData (oop) | -// | methodCounters | +// | MethodData* (metadata) | +// | MethodCounters | // |------------------------------------------------------| // | accessflags | // | vtableindex | // |------------------------------------------------------| // | resultindex (C++ interpreter only) | // |------------------------------------------------------| -// | methodsize | intrinsicid| flags | +// | methodsize | intrinsicid | flags | // |------------------------------------------------------| // | code (pointer) | // | i2i (pointer) |

On Mon, Jan 13, 2014 at 9:07 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com_ _<mailto:jesper.wilhelmsson at oracle.com> <mailto:jesper.wilhelmsson at oracle.com_ _<mailto:jesper.wilhelmsson at oracle.com>>> wrote: Hi Kris! No problem, I'll add it to the change. Please verify that it looks OK in the updated webrev (same link as before). http://cr.openjdk.java.net/~_jwilhelm/8025856/webrev.1/ <http://cr.openjdk.java.net/%7E_jwilhelm/8025856/webrev.1/> <http://cr.openjdk.java.net/~jwilhelm/8025856/webrev.1/_ _<http://cr.openjdk.java.net/%7Ejwilhelm/8025856/webrev.1/>> /Jesper Krystal Mok skrev 10/1/14 7:17 PM: Hi Jesper, Nice to see cleaner code! Hitchhike: It'd be nice if you could include this typo in: http://mail.openjdk.java.net/_pipermail/hotspot-compiler-_dev/2014-January/012944.html <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2014-January/012944.html> I didn't really like sending one-liner comment typo fix anyway...with a lot of other typos, the story is different ;-) Thanks, Kris On Fri, Jan 10, 2014 at 11:54 PM, Coleen Phillmore <coleen.phillimore at oracle.com_ _<mailto:coleen.phillimore at oracle.com> <mailto:coleen.phillimore at oracle.com_ _<mailto:coleen.phillimore at oracle.com>> _<mailto:coleen.phillimore@_ _mailto:coleen.phillimore@oracle.com <http://oracle.com> <mailto:coleen.phillimore at oracle.com_ _<mailto:coleen.phillimore at oracle.com>>>> wrote: Seems fine with me also. Could you find typos in the comments in the runtime code "by accident" too? :) thanks, Coleen On 1/10/2014 10:39 AM, Daniel D. Daugherty wrote: On 1/10/14 5:49 AM, Jesper Wilhelmsson wrote: Hi, I have a change out for review that fixes a huge pile of typos in the comments in the GC code. The RFR was sent to the GC list, but I want to give a heads up in case anyone else is changing GC code and want to avoid merge conflicts. The patch: http://cr.openjdk.java.net/~__jwilhelm/8025856/webrev.1/ <http://cr.openjdk.java.net/%7E__jwilhelm/8025856/webrev.1/> <http://cr.openjdk.java.net/~_jwilhelm/8025856/webrev.1/_ _<http://cr.openjdk.java.net/%7E_jwilhelm/8025856/webrev.1/>> <http://cr.openjdk.java.net/~_jwilhelm/8025856/webrev.1/ <http://cr.openjdk.java.net/%7E_jwilhelm/8025856/webrev.1/> <http://cr.openjdk.java.net/~jwilhelm/8025856/webrev.1/_ _<http://cr.openjdk.java.net/%7Ejwilhelm/8025856/webrev.1/>>> There are also a few files where I happened to find a few typos "by accident" in code that is not strictly GC code. These are: src/share/vm/memory/heap.cpp src/share/vm/memory/heap.hpp _src/share/vm/memory/_allocation.hpp _src/share/vm/memory/_resourceArea.hpp _src/share/vm/runtime/thread._cpp There is a total of eight typos fixed in these files so I think the risk of merge conflicts here is minimal. Are there any objections to including these fixes in the change? Vote: go for it! Dan Thanks, /Jesper

More information about the hotspot-dev mailing list