[lworld] RFR: 8357785: [lworld] TestResolvedJavaType fails due to unexpected getInstanceFields order [v2]

[Quan Anh Mai]

On Tue, 22 Jul 2025 07:49:32 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> It's all a matter of order. In short, mainline doesn't sort objects fields by offset, but just take them as they come from the stream. The order can be arbitrary, but in some places, it matters it's the same everywhere. Valhalla is sorting too much for mainline's taste, a merge caused clashes.
>> 
>> But maybe, a bit of archeology:
>> - Mainline [JDK-8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields](https://bugs.openjdk.org/browse/JDK-8350892): it removed some sorting of fields, keeping it consistent between `reassign_fields_by_klass` and `ciInstanceKlass::compute_nonstatic_fields()`: they both had fields offset-sorted, now they are both stream-sorted. Most of HotSpot agrees with this new order. This change also add a new test checking the order is consistent: `compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java`.
>> 
>> - Valhalla: [#1429](https://github.com/openjdk/valhalla/pull/1429) brings [JDK-8350892](https://bugs.openjdk.org/browse/JDK-8350892) in Valhalla. This merge is a bit mysterious to me,  how some sorting reappeared in Valhalla-untouched code. For instance, [JDK-8350892](https://bugs.openjdk.org/browse/JDK-8350892) removed the function `
>> static int sort_field_by_offset(ciField** a, ciField** b)` and
>>   ```cpp
>>   // Now sort them by offset, 
>>   // (In principle, they could mix with superclass fields.)
>>   fields->sort(sort_field_by_offset);
>>   ```
>>   from `int ciInstanceKlass::compute_nonstatic_fields()` (in `ciInstanceKlass.cpp`), but the merge only removes the comment, but leave the `->sort()` and `sort_field_by_offset`. My guess is that it was a tentative fix for some failing tests after merge. Nevertheless `compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/VirtualObjectDebugInfoTest.java` is ProblemListed.
>> 
>> - Valhalla: After this merge, Valhalla continues to assume some ordering, as one can see in [JDK-8355076: [lworld] Incorrect declared_nonstatic_fields computation](https://bugs.openjdk.org/browse/JDK-8355076) that (among other things), makes a new occurrence of `->sort(sort_field_by_offset)`, to be consistent with `InlineKlass::collect_fields`. But now, everything is different! The new test introduced by [JDK-8350892](https://bugs.openjdk.org/browse/JDK-8350892) connects more things together, and enforce the same order in `ciInstanceKlass::compute_nonstatic_fields` and in `Class.getDeclaredFields` (reflexion related, I think. I don't know that ...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address Quan Anh's comments

LGTM, I have some small suggestions.

src/hotspot/share/oops/fieldStreams.hpp line 210:

> 208: class HierarchicalFieldStreamBase : public StackObj {
> 209: protected:
> 210:   virtual FieldStreamType& current_stream () = 0;

Small style: there should not be a space between the method name and the parenthesis. Also, I think these can be `private`.

src/hotspot/share/oops/fieldStreams.hpp line 277:

> 275:  */
> 276: template<typename FieldStreamType>
> 277: class HierarchicalFieldStream : public HierarchicalFieldStreamBase<FieldStreamType>  {

1. It's good practice marking a class as `final` so the compiler can do more efficient devirtualization.
2. It seems the `HierarchicalFieldStreamBase` is only for code reuse. Then it would be better to use private inheritance instead.

src/hotspot/share/oops/fieldStreams.hpp line 320:

> 318: 
> 319:  public:
> 320:   HierarchicalFieldStream(InstanceKlass* klass) :

While you are at it, this could be `explicit`, too.

src/hotspot/share/oops/fieldStreams.hpp line 342:

> 340:  * care about static fields, we restrict it to regular non-static fields.
> 341:  */
> 342: class TopDownHierarchicalNonStaticFieldStreamBase : public HierarchicalFieldStreamBase<JavaFieldStream> {

Same with this child of `HierarchicalFieldStreamBase`

src/hotspot/share/runtime/signature.hpp line 588:

> 586:     : _bt(T_ILLEGAL), _offset(-1), _symbol(nullptr) {}
> 587: 
> 588:   SigEntry(BasicType bt, int offset, Symbol* symbol, bool null_marker)

Ah `symbol` here should be the name, not the signature. Could you at least rename the parameter name here? Could we rename the field name, too?

-------------

Marked as reviewed by qamai (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1511#pullrequestreview-3048615246
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2226389572
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2226384669
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2226391619
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2226386450
PR Review Comment: https://git.openjdk.org/valhalla/pull/1511#discussion_r2226403068