Changing lots of files - mainly GC code

Krystal Mok rednaxelafx at gmail.com
Tue Jan 14 15:21:36 PST 2014


No problem with that either. :-)
Thanks, Coleen!

- Kris


On Wed, Jan 15, 2014 at 4:40 AM, Coleen Phillimore <
coleen.phillimore at oracle.com> wrote:

>
> 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> 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> 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 method_size 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>,
>>>>
>>>>       //    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>
>>>>
>>>>     //      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 method_size first for better gc cache locality.
>>>> -//
>>>>   // The actual bytecodes are inlined after the end of the Method
>>>> struct.
>>>>   //
>>>>   // There are bits in the access_flags telling whether inlined tables
>>>> are present.
>>>> @@ -64,17 +62,17 @@
>>>>   // | header                                               |
>>>>   // | klass                                                |
>>>>   // |------------------------------------------------------|
>>>> -// | ConstMethod*                   (oop)                 |
>>>> +// | ConstMethod*                   (metadata)            |
>>>>   // |------------------------------------------------------|
>>>> -// | methodData                     (oop)                 |
>>>> -// | methodCounters                                       |
>>>> +// | MethodData*                    (metadata)            |
>>>> +// | MethodCounters                                       |
>>>>   // |------------------------------------------------------|
>>>>   // | access_flags                                         |
>>>>   // | vtable_index                                         |
>>>>   // |------------------------------------------------------|
>>>>   // | result_index (C++ interpreter only)                  |
>>>>   // |------------------------------------------------------|
>>>> -// | method_size             |   intrinsic_id|   flags    |
>>>> +// | method_size             | intrinsic_id  |   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>>
>>>> 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/~jwilhelm/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>>> 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/~__jwilhelm/8025856/webrev.1/>
>>>>
>>>>
>>>>                      <
>>>> http://cr.openjdk.java.net/~__jwilhelm/8025856/webrev.1/
>>>>         <http://cr.openjdk.java.net/~jwilhelm/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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140115/50e1f1ad/attachment-0001.html 


More information about the hotspot-compiler-dev mailing list