RFR(S): 8245021: Add method 'remove_if_existing' to growableArray.

[Patric Hedlin]

Hi,

On 2020-05-19 21:03, Liu, Xin wrote:
> Hi, Patric,
>
> I don't object to your change.  I feel that the API 'remove' of GrowableArray was not good.  Even though it's complexity is still linear, you scan all elements and write some of them.
The interface (remove) is what it is I guess. There was no intention to 
change current behaviour.

> The problem is it has to retain order. Actually, I didn't run into any problem when I replace the removing element with the last one.
> It suggests that probably nobody in hotspot makes use the sorted GrowableArray.
>
> I found another interesting point.  There's an API delete_at which ignore orders, so I try and replace your remove_if_exists with it.
>    bool delete_if_existing(const E& elem) {
>      int index = find(elem);
>
>      if (index != -1) {
>        _data[index] = _data[--_len];
>        return true;
>      }
>
>      return false;
>    }
> I didn't have any regression in jtreg:hotspot:tier1.  Actually, CodeCache::unregister_old_nmethod use the same trick.
Indeed, in analogy with *remove*, you might argue that both "a delete" 
and delete_if_existing are missing in the interface (in cases when order 
is not required). Perhaps also; delete_all/remove_all for multi entry 
usage. However, adding one or the other is perhaps another RFE (and 
might require more than a test-run to replace current uses of 
*remove*).At the same time, I have to assume that they have not been 
added for a reason.

I moved this to 16 (after JDK-8247755). Added some refactoring to new 
webrev (refreshed).

Best regards,
Patric

> Here is current implementation of delete_at().  It checks if the index is the last element, and skip copying if so.  I am not sure if an extra comparison is worthy here.
> Users should use pop() instead in that scenario.
>    
>    // The order is changed.
>    void delete_at(int index) {
>      assert(0 <= index && index < _len, "illegal index");
>      if (index < --_len) {
>        // Replace removed element with last one.
>        _data[index] = _data[_len];
>      }
>    }
>
> Thanks,
> --lx
>
>
>      Dear all,
>
>      I would like to ask for help to review the following change/update:
>
>      Issue:  https://bugs.openjdk.java.net/browse/JDK-8245021
>      Webrev: http://cr.openjdk.java.net/~phedlin/tr8245021/
>
>
>      8245021: Add method 'remove_if_existing' to growableArray.
>
>      Minor improvement to simplify the code pattern "if contains then remove"
>      found in a few places (in "compile.hpp").
>
>
>      Testing: hs-tier1-3
>
>
>      Best regards,
>      Patric