Changing lots of files - mainly GC code
Krystal Mok
rednaxelafx at gmail.com
Tue Jan 14 11:29:21 PST 2014
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.
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/dad984e0/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list