RFR: 8342011: Conditionally compile ReservedHeapSpace compressed heap support [v2]

Stefan Karlsson stefank at openjdk.org
Wed Oct 16 08:04:11 UTC 2024


On Wed, 16 Oct 2024 07:54:48 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this change to ReservedHeapSpace to conditionally include the
>> support for compressed oops only in 64bit builds.  This code is unused and
>> inapplicable in 32bit builds.  Further, it contains code that is problematic
>> in a 32bit build.  See the JBS issue for details.
>> 
>> Testing: mach5 tier1, GHA testing
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
> 
>   improve conditionalization

Changes requested by stefank (Reviewer).

src/hotspot/share/memory/virtualspace.cpp line 646:

> 644: #ifndef _LP64
> 645:     NOT_LP64(ShouldNotReachHere();)
> 646: #else

Suggestion:

#ifndef _LP64
    ShouldNotReachHere();
#else

With that said, I think it would be nicer to invert the code:

#ifdef _LP64
  ... All the code we typically care of up front
#else
  ... Code we typically ignore
  ShouldNotReachHere();
#endif

src/hotspot/share/memory/virtualspace.hpp line 138:

> 136: 
> 137:   // Compressed oop support is not relevant in 32bit builds.
> 138: #ifdef _LP64

Suggestion:


  // Compressed oop support is not relevant in 32bit builds.
#ifdef _LP64


The `#endif // _LP64 below have blank lines before and after, so it seems more consistent to also have it here.`

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

PR Review: https://git.openjdk.org/jdk/pull/21484#pullrequestreview-2371525505
PR Review Comment: https://git.openjdk.org/jdk/pull/21484#discussion_r1802558014
PR Review Comment: https://git.openjdk.org/jdk/pull/21484#discussion_r1802560318


More information about the hotspot-runtime-dev mailing list