Changing lots of files - mainly GC code
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
>> 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>
>> <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 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>
>> <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
>>
>>
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140114/c8fd077f/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list