RFR[L]: 8237767 Field layout computation overhaul
Frederic Parain
frederic.parain at oracle.com
Fri Jan 24 17:20:15 UTC 2020
Hi David,
> On Jan 24, 2020, at 08:19, David Holmes <david.holmes at oracle.com> wrote:
>
> Hi Fred,
>
> This is quite a mini-project. :)
Yes, indeed!
>
> On 24/01/2020 1:03 am, Frederic Parain wrote:
>> Greetings,
>> Please review this change proposing a new code to compute field layouts.
>> CR: https://bugs.openjdk.java.net/browse/JDK-8237767
>> webrev: http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/
>> The CR includes a detailed description of the motivation and the implementation.
>
> Thanks for the detailed discussion. I can't comment on the details. This is enabling technology for Valhalla so as long as it is functionally correct without significant performance impact, then it seems good.
>
> I have a number of comments, mainly code style issues and typos etc.
Thank you for the detailed review, I’ve fixed all issues you mentioned and
my answers to your questions are inlined below.
The new webrev is available here:
http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.05/index.html
Rerunning tier1-3 right now.
>
> Thanks,
> David
> -----
>
> src/hotspot/share/ci/ciInstanceKlass.cpp
>
> 216 assert(self->is_loaded(), "must be loaded to field info");
>
> Seems to be a word missing in the message - "access field info” ?
Fixed
>
> 465 int super_fsize = super->nonstatic_field_size() * heapOopSize;
>
> Seems to be an unused local variable now.
Removed.
>
> 466 int super_flen = super->nof_nonstatic_fields();
>
> Could be folded directly into the assert so we don't call in product.
Calling not_nonstatic_fields() has the side effect to compute non-static fields,
which is required to get a correct value when reading super->_nonstatic_fields,
so the call is needed even in product builds.
>
> ---
>
> src/hotspot/share/ci/ciInstanceKlass.hpp
>
> 229 bool contains_field_offset(int offset) {
> 230 fieldDescriptor fd;
> 231 return this->get_instanceKlass()->find_field_from_offset(offset, false, &fd);
> 232 }
>
> Seems odd to fill in the fieldDescriptor but not use it at all. Suggests to me that instanceKlass may need a similar query for contains_field_offset. Oh it does - which has the same problem but at least the above code could be:
>
> return get_instanceKlass()->contains_field_offset(offset);
>
> Style nit: no need for this->
Fixed
>
> ---
>
> src/hotspot/share/classfile/classFileParser.cpp
>
> 3935 OopMapBlocksBuilder::OopMapBlocksBuilder(unsigned int max_blocks, TRAPS) {
>
> Style nit: extra space before max_blocks.
Fixed
>
> General style issue: when breaking a long line with a method call, the new line (containing arguments) should be indented to the opening ( of the method call e.g.
>
> 3941 nonstatic_oop_maps = NEW_RESOURCE_ARRAY_IN_THREAD(
> 3942 THREAD, OopMapBlock, max_nonstatic_oop_maps);
>
> should be
>
> 3941 nonstatic_oop_maps = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD,
> 3942 OopMapBlock, max_nonstatic_oop_maps);
>
> or alternatively something like:
>
> 3941 nonstatic_oop_maps =
> 3942 NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, OopMapBlock, max_nonstatic_oop_maps);
>
> depending on line length and number of lines.
Fixed
>
> Second example:
>
> 3954 assert(nof_blocks && nonstatic_oop_map_count == 0 &&
> 3955 nof_blocks <= max_nonstatic_oop_maps, "invariant");
>
> should be:
>
> 3954 assert(nof_blocks && nonstatic_oop_map_count == 0 &&
> 3955 nof_blocks <= max_nonstatic_oop_maps, "invariant”);
Fixed
>
> etc. This applies across all files.
Fixes applied lines 4003, 4011, 4041, 4138, 4143.
>
> --
>
> Style nit:
>
> 4137 int super_oop_map_count = (_super_klass == NULL) ? 0 :_super_klass->nonstatic_oop_map_count();
> 4138 int max_oop_map_count =
> 4139 super_oop_map_count +
> 4140 fac->count[NONSTATIC_OOP];
>
> Given the length of 4137, splitting 4138 over three lines seems odd. At most you want two lines.
Fixed
>
> ---
>
> src/hotspot/share/oops/instanceKlass.hpp
>
> You need to be careful with _extra_flags usage if there can be concurrently updated bits. At the moment it looks like redefinition is a mutable dynamic property, whilst "contended annotations" should be a static immutable property - is that right?
Correct, _has_contended_annotations is a static immutable property, while _is_being_redefined is a mutable one.
>
> 761 return (_extra_flags & _extra_has_contended_annotations);
>
> Style nit: use of implicit boolean, please change to:
>
> 761 return (_extra_flags & _extra_has_contended_annotations != 0);
>
> Ditto for line 774.
Fixed
>
> ---
>
> src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java
>
> 714 public int compare(ResolvedJavaField a, ResolvedJavaField b)
> 715 {
>
> Style nit: opening brace should be on the same line.
Fixed
>
> 746 Arrays.sort(result, new SortByOffset());
>
> No need to create a comparator on every call. Declare a static instance in the SortByOffset class. Ditto line 808.
Fixed
>
> I have to wonder about the performance of this code now that it has to do a linear search and post-sort the results? (But that's for the compiler folk to raise. :) ).
The Graal team already had a look at this code (the new field layout had an impact on some Graal unit
tests, so I’ve discussed the issue with them and tests have been fixed), and even if this not an
official review yet, they didn’t express strong objections. I’ll double check with them again before
pushing the code.
>
> ---
>
> src/hotspot/share/classfile/fieldLayoutBuilder.hpp
>
> 41 // All RawBlock must have a size and a alignment.
>
> s/RawBlocks/RawBlocks/
> s/and a/and an/
Fixed
>
> 42 // exact size of the field expressed in bytes. The alignment is
> 43 // the alignment constraint of the field (1 for byte, 2 for short,
> 44 // 4 for int, 8 for long, etc.)
>
> I thought we could have stronger alignment constraints than that. For example if an int field is subject to atomic updates via CAS then depending on platform it may need to be 64-bit aligned? Or are we lucky that all (both :) ) of our platforms with hardware cas support direct 32-bit cas? (Good job we don't support AtomicByte or AtomicShort :) ).
There’s no such stronger alignment constraints in the old field layout code.
Fortunately, the new code allows adding such constraints if needed (it was designed
to support hyper-alignment in the future).
>
> 53 class LayoutRawBlock : public ResourceObj {
>
> Why is the class LayoutRawBlock when you refer to plain RawBlock when describing things? Is there some other kind of RawBlock that will come in with inline types?
During a code walk through last week, Lois and Harold suggested that “RawBlock” was too generic
and could cause name conflicts, so it has been changed to “LayoutRawBlock”. Unfortunately, the
refactoring didn’t apply automatically to comments, I’ve fixed that.
There’s no other kind of data structures coming in with Valhalla, only new methods and
additional cases in existing methods.
>
> 61 FLATTENED, // flattened field
>
> Does this have any meaning before inline types come in?
Yes, I wanted to reserved the entry in the enum.
>
> 205 // and two classes with hard coded offsets (java,lang.ref.Reference
>
> s/two/two for/
Fixed
>
> 207 // is that each kind of classes has a different set goals regarding
>
> s/classes/class/
Fixed
>
> 215 // 2 - Field sorting: fields are sorted out according to their
>
> s/out//
Fixed
>
> 220 // 4 - Epilogue: oopmaps are generated, layout information are
>
> s/are/is/
Fixed
>
> 221 // prepared so other VM components can use them (instance size,
>
> s/them/it/
Fixed
>
> ---
>
> src/hotspot/share/classfile/fieldLayoutBuilder.cpp
>
> 2 * Copyright (c) 2019, 2019, Oracle and/or its affiliates. All rights reserved.
>
> Copyright format is incorrect but will be okay when the second 2019 is changed to 2020. :)
Fixed with a single year format in .cpp and .hpp and test files.
>
> In FieldLayoutBuilder::epilogue you have a number of calls to Thread::current() as well as an implicit call when you use ResourceMarks. You should capture the current thread once in a local and reuse it.
Fixed
>
> ---
>
> That's it.
Thank you,
Fred
>
>
>> The current version keeps the old code accessible (with a VM flag) in case the
>> small changes in computed layouts cause troubles to some applications or tools.
>> The goal is to get rid of this old code, preferably as soon as possible.
>> Testing tier1-8.
>> Thank you,
>> Fred
More information about the hotspot-dev
mailing list