Changing lots of files - mainly GC code
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Jan 14 10:46:36 PST 2014
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/0a67943d/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list