RFR[L]: 8237767 Field layout computation overhaul

Fri Jan 24 21:21:04 UTC 2020

One comment below on David's comments.

On 1/24/20 8:19 AM, David Holmes wrote:
> 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.

I had a look at this because I had originally added _is_being_redefined 
thinking it would be something that multiple threads tested and should 
be volatile and have memory ordering.  So that's why it had it's own 
bool field.  Looking again, all threads access this inside of the 
RedefineClasses_lock so it's okay for this to be a non-atomic bit field.

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