RFR[L]: 8237767 Field layout computation overhaul
Frederic Parain
frederic.parain at oracle.com
Tue Jan 28 21:30:18 UTC 2020
Coleen,
Thank you for this second review:
> On Jan 28, 2020, at 13:57, coleen.phillimore at oracle.com wrote:
>
>
> Fred, Some more minor comments.
>
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.06/src/hotspot/share/classfile/classFileParser.cpp.udiff.html
>
> +#include <classfile/fieldLayoutBuilder.hpp>
> This has brackets.
Fixed.
>
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.06/src/hotspot/share/classfile/classFileParser.hpp.udiff.html
>
> OopMapBlockBuilder and FieldLayoutInfo field names should probably start with _ (preexisting problem but since you moved it, it should follow the coding style).
>
Fixed.
> +#include "oops/instanceKlass.hpp"
>
> This is out of alphabetical order.
>
Fixed
>
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.06/src/hotspot/share/classfile/fieldLayoutBuilder.cpp.html
>
> My comment about TRAPS/CHECK also applied to compute_regular_layout, compute_java_lang_ref_Reference_layout, compute_boxing_class_layout and build_layout too. None of these throw C++ exceptions and appear to need a THREAD parameter passed in.
>
I missed the scope of your comment in your first review.
Fixed now, none of the methods in fieldLayoutBuilder.[ch]pp is using TRAPS/CHECK now.
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.06/test/hotspot/jtreg/runtime/FieldLayout/FieldDensityTest.java.html
>
> I think this needs @bug 8237767
Added.
New webrev with changes above and updated copyright years:
http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.07/index.html
Thank you,
Fred
>
> On 1/28/20 9:09 AM, Frederic Parain wrote:
>> Coleen,
>>
>> Thank you for reviewing this change.
>>
>> New webrev:
>>
>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.06/index.html
>>
>>
>> My answers are inlined below.
>>
>>
>>> On Jan 24, 2020, at 16:17, coleen.phillimore at oracle.com
>>> wrote:
>>>
>>>
>>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/ci/ciInstanceKlass.hpp.udiff.html
>>>
>>>
>>> bool contains_field_offset(int offset) {
>>> - return instanceOopDesc::contains_field_offset(offset, nonstatic_field_size());
>>> + fieldDescriptor fd;
>>> + return this->get_instanceKlass()->find_field_from_offset(offset, false, &fd);
>>> }
>>>
>>>
>>> This has to go into the VM if it's going to access metadata, with VM_ENTRY_MARK, so probably belongs in the cpp file. Also, why doesn't this call contains_field_offset() from InstanceKlass? from here:
>>>
>>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/oops/instanceKlass.cpp.udiff.html
>>>
>>>
>>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/oops/instanceKlass.hpp.udiff.html
>> Good catch! I’ve changed the code to:
>>
>> bool ciInstanceKlass::contains_field_offset(int offset) {
>> VM_ENTRY_MARK;
>> return get_instanceKlass()->contains_field_offset(offset);
>> }
>>
>>
>>> + public:
>>> + enum {
>>> + _extra_is_being_redefined = 1 << 0, // used for locking redefinition
>>> + _extra_has_contended_annotations = 1 << 1 // has @Contended annotation
>>> + };
>>> +
>>>
>>>
>>> Why is this enum public? Also, I think you should make these misc_flags and make _flags a u4. There's already an alignment gap in InstanceKlass and we can file an RFE to fix that after this change.
>>>
>> I’ve extended _misc_flags to a u4 and moved all flags to it.
>> Note: the type change impacts vmStruct and JVMCI.
>>
>
> Great. I'll file a new bug to reduce the alignment gaps.
>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/runtime/globals.hpp.udiff.html
>>>
>>>
>>> I think you should make UseNewLayout a diagnostic flag since we would like to remove the old code once it has gotten more testing. The only reason someone would use it would be to diagnose some differing behavior.
>>>
>> Done.
>>
>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java.udiff.html
>>>
>>>
>>> Someone who knows the compilers should have a peek at this.
>>>
>>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/classfile/fieldLayoutBuilder.hpp.html
>>>
>>>
>>> 37 // A RawBlock describes an element of a layout.
>>> 38 // Each field is represented by a RawBlock.
>>>
>>> Thank you for changing RawBlock to LayoutRawBlock. Can you update the comments here and the description in the RFE?
>>>
>> Comments and RFE fixed.
>>
>>
>>> 29 #include "classfile/classLoaderData.inline.hpp"
>>>
>>>
>>> .hpp files shouldn't include .inline.hpp files. Whatever uses the inline code should go into the cpp file (or if critical, add an inline.hpp file).
>>>
>> Fixed.
>>
>>
>>> 132 static const int INITIAL_LIST_SIZE;
>>>
>>>
>>> It's odd to see this without an initialization.
>>>
>> I moved the initialization from the .cpp to the .hpp file.
>>
>>
>>> 227 class FieldLayoutBuilder : public ResourceObj {
>>>
>>>
>>> FieldLayoutBuilder is a StackObj isn't it?
>>>
>>> It might be nice to line up the fields, which is part of the coding standard that I only somewhat agree on. Here it would enhance readability.
>>>
>> Done.
>>
>>
>>> 264 protected:
>>>
>>>
>>> Why are these functions "protected"? I don't see anything that inherits from FieldLayoutBuilder that might want to call these functions and not the other functions.
>>>
>> It’s a left over from a previous design.
>> Changed to private.
>>
>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/classfile/fieldLayoutBuilder.cpp.html
>>>
>>>
>>> 544 _contended_groups = new (ResourceObj::RESOURCE_AREA, mtInternal) GrowableArray<FieldGroup*>(8);
>>>
>>>
>>> Growable arrays are default ResourceObj, so I think the 'new' arguments aren't needed (there were a couple of these).
>>>
>> I’ve changed _contended_groups’ type to GrowableArray<FieldGroup*> instead of GrowableArray<FieldGroup*>*
>> It removes the “new” and simplifies the management of the _contended_groups field.
>>
>>
>>> 698 OopMapBlocksBuilder* nonstatic_oop_maps =
>>> 699 new OopMapBlocksBuilder(max_oop_map_count, Thread::current());
>>>
>>> The code uses OopMapBlocksBuilder as a StackObj, which seems to make more sense. Can you make it StackObj and not have it allocate from resourceArea?
>>>
>> nonstatic_oop_maps is not used as StackObj, but as a ResourceObj, it escapes the method at the end,
>> when the FieldLayoutInfo is filled.
>>
>
> Ok.
>>
>>> 531 void FieldLayoutBuilder::regular_field_sorting(TRAPS) {
>>>
>>> 635 regular_field_sorting(CHECK);
>>>
>>> None of these functions throw Java class exceptions. They shouldn't pass TRAPS and CHECK which are for Java exceptions. If you want to avoid JavaThread::current, you can pass JavaThread* thread as the first parameter.
>>>
>> Fixed
>>
>>
>>> I think for this change, it might be better to let this:
>>>
>>> + OopMapBlock* oop_maps_copy = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, OopMapBlock,
>>>
>>> be
>>>
>>> + OopMapBlock* oop_maps_copy = NEW_RESOURCE_ARRAY(OopMapBlock,
>>>
>>>
>>> and get it's own Thread::current(). I don't think it saves many instructions and definitely not any time.
>>>
>> Fixed
>>
>>
>>> +void OopMapBlocksBuilder::compact(TRAPS) {
>>>
>>>
>>> This shouldn't take a thread argument, so shouldn't be declared with TRAPS, see above.
>>>
>> Fixed
>>
>>
>>> 748 if (PrintFieldLayout) {
>>>
>>>
>>> In some future change, this should be turned into unified logging.
>>>
>> Regarding the complexity of the task (considering the amount of data that can be generated
>> and the variations in the output format), I’d prefer to address this conversion in a separate
>> changeset.
>>
>
> Definitely should be a follow-up RFE. This was one Print* flag that we weren't sure if we wanted to convert in the first pass, so it should be looked at on its own.
>
>>
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/classfile/classFileParser.cpp.udiff.html
>>>
>>>
>>> Why was AnnotationCollector moved to the header file? It seems only used by classFileParser.cpp?
>>>
>>
>> I needed it here during some steps of the development, but this move is not required anymore,
>> I moved the class back to the .cpp file.
>>
>
> Great.
>>
>>> This is a really nice change and your comments are really good. These are all pretty small comments.
>>>
>>>
>> Thank you for this in depth review.
>
> Thanks for making the changes.
>
> Coleen
>
>>
>> Fred
>>
>>
>>> Thanks,
>>> Coleen
>>>
>>> On 1/23/20 10: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.
>>>>
>>>> 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