RFR: 8353175: Eliminate double iteration of stream in FieldDescriptor reinitialization [v6]

[Johan Sjölen]

On Thu, 10 Apr 2025 11:41:17 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> src/hotspot/share/oops/instanceKlass.cpp line 1945:
>> 
>>> 1943:     fields_sorted.sort(compare_fields_by_offset);
>>> 1944:     fieldDescriptor fd;
>>> 1945:     for (auto it = fields_sorted.begin(); it != fields_sorted.end(); ++it) {
>> 
>> Ah, that's not what I meant :) There is no need to use iterators here, just pull `length` out of `fields_stored.length()`, and use the same old indexed loop:
>> 
>> 
>>  int length = fields_stored.length();
>>  if (length > 0) {
>>     fields_sorted.sort(compare_fields_by_offset);
>>     for (int i = 0; i < length; i++) {
>>       ...
>
> I know you haven't asked specifically for iterators, but to me this seems 'the' cleaner way, rather than plain old indexed loop. Or do you have doubts about this being inlined to effectively the same code?

I think using an iterator is fine, that's up to you. I am a bit bothered by having this change also slip in a change to the `GrowableArray` API, however. Anyone who wants to review that part of the code will miss that change.

I'd like to suggest that you change to an indexed for loop, and you make a follow-up PR with your GA changes and where you switch the loop.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24290#discussion_r2037179686