RFR(S): 8164230: Convert TestCodeCacheRemSet_test to GTest

Mikael Gerdin mikael.gerdin at oracle.com
Fri Aug 26 08:11:49 UTC 2016

Hi Kirill,

On 2016-08-22 20:40, Kirill Zhaldybin wrote:
> Mikael,
> Thank you for reviewing the fix!
> Here are a new WebRev:
> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164230/webrev.01/

A few nits in g1CodeRootSetTable:
* the include guard still says _INTERNAL_HPP
* even though it sucessfully compiles right now I'd prefer if the header 
forward declares or includes its obvious dependencies, in this case 
those would just be a forward-declaration of "class nmethod"

In test_g1CodeCacheRemSet
   size_t threshold(G1CodeRootSet* root_set) {
     return root_set->Threshold;

even though this compiles I would prefer if you referred to this static 
member through the class name:
That way you can avoid passing the instance as a parameter as well.

Otherwise I think this change looks good.


> Changes:
> 1. g1CodeCacheRemSet.internal.hpp renamed to g1CodeRootSetTable.hpp and
> the class was renamed to G1CodeRootSetTable, #include  added and I was
> able to build images and gtests with --disable-precompiled-headers.
> 2. Moved G1CodeRootSet to the test method
> 3. Renamed getters in G1CodeRootSetTest to snake_case.
> Could you please let me know your opinion?
> Thank you.
> Regards, Kirill
> On 22.08.2016 16:43, Mikael Gerdin wrote:
>> Hi Kirill,
>> On 2016-08-22 14:57, Kirill Zhaldybin wrote:
>>> Dear all,
>>> Could you please review this fix for 8164230?
>>> Since we need CodeRootSetTable class to be declared in the test it was
>>> moved to new file src/share/vm/gc/g1/g1CodeCacheRemSet.internal.hpp.
>>> This file is included by both test_g1CodeCacheRemSet.cpp and
>>> g1CodeCacheRemSet.cpp.
>> Please name the new file g1CodeRootSetTable.hpp and rename the class
>> G1CodeRootSetTable instead of inventing a new ".internal.hpp" format.
>> Also, the new hpp file does not properly #include its dependencies and
>> therefore the build fails if precompiled headers are disabled, please
>> add the appropriate includes and forward declarations in order to make
>> it complete.
>> The class G1CodeRootSetTest has a bunch of getters and a G1CodeRootSet
>> member, why are they like that instead of allocated on the stack like
>> the original version of the test?
>> /Mikael
>>> G1CodeRootSetTest is a friend class for G1CodeRootSet so we can get
>>> access to its internals.
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8164230
>>> WebRev:
>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164230/webrev.00/
>>> Thank you.
>>> Regards, Kirill

More information about the hotspot-gc-dev mailing list