RFR[L]: 8237767 Field layout computation overhaul

[David Holmes]

Hi Fred,

This is quite a mini-project. :)

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.

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" ?

465     int super_fsize  = super->nonstatic_field_size() * heapOopSize;

Seems to be an unused local variable now.

  466     int super_flen   = super->nof_nonstatic_fields();

Could be folded directly into the assert so we don't call in product.

---

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->

---

src/hotspot/share/classfile/classFileParser.cpp

3935 OopMapBlocksBuilder::OopMapBlocksBuilder(unsigned int  max_blocks, 
TRAPS) {

Style nit: extra space before max_blocks.

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.

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");

etc. This applies across all files.

--

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.

---

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?

  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.

---

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.

  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.

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. :) ).

---

src/hotspot/share/classfile/fieldLayoutBuilder.hpp

  41 // All RawBlock must have a size and a alignment.

s/RawBlocks/RawBlocks/
s/and a/and an/

   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 :) ).

  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?

   61     FLATTENED,     // flattened field

Does this have any meaning before inline types come in?

  205 // and two classes with hard coded offsets (java,lang.ref.Reference

s/two/two for/

207 // is that each kind of classes has a different set goals regarding

s/classes/class/

215 //   2 - Field sorting: fields are sorted out according to their

s/out//

220 //   4 - Epilogue: oopmaps are generated, layout information are

s/are/is/

221 //       prepared so other VM components can use them (instance size,

s/them/it/

---

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. :)

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.

---

That's it.


> 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
>