RFR: 8292170: Convert CodeRootSetTable to use ResourceHashtable [v8]

[Coleen Phillimore]

On Tue, 24 Jan 2023 13:36:45 GMT, Leo Korinth <lkorinth at openjdk.org> wrote:

>> I am replacing the old hash table with ResourceHashtable. In the process I am also removing the redundant `_length` field. By removing the `_length` field, a lot of asserts can be removed as the length will trivially match the length of the underlying table.
>> 
>> I would like to have feedback on the addition of `unlink_destruct(Function&& should_remove)`. I added it because I prefer to use a functor object that can be used by lambdas instead of an iterator object. However, I did add code to "destruct" resource objects when they are removed instead of just letting them be to reflect that we use `delete` on c-heap objects. Maybe I should remove this "improvement" and then maybe implement the function by calling the existing unlink?
>
> Leo Korinth has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove the purge list and use the resizing of the resource hash
>   table.
>   
>   Add asserts ensuring that we do not mutatet a hash table while we
>   iterate over it. Renaming .*g1CodeCacheRemSet.?pp -> .*g1CodeRootSet.?pp

This looks really good! A couple of include file cleanups needed.

src/hotspot/share/gc/g1/g1CodeRootSet.cpp line 36:

> 34: #include "runtime/atomic.hpp"
> 35: #include "services/memTracker.hpp"
> 36: #include "utilities/hashtable.inline.hpp"

Need to remove this include since it's no longer used.  Also, I don't see memTracker used either.  is stack.inline.hpp used ?

src/hotspot/share/gc/g1/g1CodeRootSet.hpp line 30:

> 28: #include "utilities/globalDefinitions.hpp"
> 29: 
> 30: #include "code/codeCache.hpp"

is codeCache.hpp used by the header file?

src/hotspot/share/gc/g1/g1CodeRootSet.hpp line 35:

> 33: #include "utilities/resourceHash.hpp"
> 34: 
> 35: class CleanCallback;

Do you need this forward declaration since it's in the .cpp file before the caller?

src/hotspot/share/gc/g1/heapRegionRemSet.cpp line 124:

> 122:           BOOL_TO_STR(_m.owned_by_self()), BOOL_TO_STR(Thread::current()->is_VM_thread()));
> 123: 
> 124:   if (!_code_roots.contains(nm)) { // with this test, we can assert that we do not modify the hash table while iterating over it

But you have the assert in 'add' too, so put_if_absent is more efficient than calling contains first.

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

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11675