RFR: JDK-8260200: optimize FreeRegionList::remove_starting_at by removing u…

[Albert Mingkun Yang]

On Thu, 21 Jan 2021 11:12:18 GMT, Hamlin Li <mli at openjdk.org> wrote:

> optimize FreeRegionList::remove_starting_at by removing unnecessary reading and setting
> 
> FreeRegionList::remove_starting_at(...) traverses from a node and removes subsequent N nodes from free list. But when traverses the free list, it removes nodes one by one by setting the prev and next pointers of prev and next node. it's not necessary do these settings for every node, as we can remove target nodes at once and just set prev and next pointers for just 2 nodes.

Changes requested by ayang (Author).

src/hotspot/share/gc/g1/heapRegionSet.cpp line 250:

> 248:   } else {
> 249:     assert_free_region_list(_tail != curr, "invariant");
> 250:   }

I think it's best to use `first` instead of `curr` in this part, since `first` is const while `curr` is not as we iterate through the list.

`prev` is const, right? How about enforcing it in the type?

src/hotspot/share/gc/g1/heapRegionSet.cpp line 261:

> 259:       assert_free_region_list(_tail != curr, "invariant");
> 260:     }
> 261:     assert(count < num_regions,

Since you are touching this area, `assert(count < num_regions)` can be dropped. (We are inside the while-loop; this condition must hold.) A more useful assert is sth like `length() >= num_regions`, if I understand the original code correctly. A bit surprised to not this mentioned in the doc.

src/hotspot/share/gc/g1/heapRegionSet.cpp line 281:

> 279:   }
> 280: 
> 281:   if (prev == NULL) {

A brief doc on what `prev` and `next` point to could be nice; sth like: `prev` points to the node right before `first` or null when `first == _head`, etc.

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

PR: https://git.openjdk.java.net/jdk/pull/2181