RFR: 8253555: Make ByteSize and WordSize typed scoped enums

[Kim Barrett]

> On Sep 24, 2020, at 2:29 AM, Stefan Karlsson <stefank at openjdk.java.net> wrote:
> 
> It's a common bug in HotSpot that byte values are used when the code asks for words, and vice versa. One attempt to
> prevent that has been to use the classes ByteSize and WordSize, to make the compiler detect these problems. The current
> implementation is a bit problematic, because both types are int typedefs in release builds, and wrapper classes in
> debug builds. This means that we can't use these types in overload resolution. I propose that we fix that by changing
> the type to scoped enums with a fixed int type. The compiler will then be apple to completely separate ByteSize,
> WordSize, and ints. We also don't need different code in release vs debug builds.
> 
> FWIW, I once created a size_t versions of these classes and annotated all the metaspace code with these types. This
> experiment flushed out a handful of bugs (some of them were known and the reason for trying that experiment)
> 
> There are some controversy about having these classes around. See for example:
> JDK-8041956: remove ByteSize and WordSize classes
> 
> I hope we can stay away from that discussion in this PR.
> 
> -------------
> 
> Commit messages:
> - 8253555: Make ByteSize and WordSize typed scoped enums
> 
> Changes: https://git.openjdk.java.net/jdk/pull/328/files
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=328&range=00
>  Issue: https://bugs.openjdk.java.net/browse/JDK-8253555
>  Stats: 188 lines in 8 files changed: 17 ins; 157 del; 14 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/328.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/328/head:pull/328
> 
> PR: https://git.openjdk.java.net/jdk/pull/328

I've also experimented with various changes to these classes, eventually
concluding I should wait for scoped enums. This looks exactly like what I
had in mind. The alternative of a thin wrapper class may have unpleasant
costs on some platforms due to ABI issues.

------------------------------------------------------------------------------
src/hotspot/share/utilities/sizes.hpp 

I think all the operations here should be constexpr rather than just inline.

------------------------------------------------------------------------------
src/hotspot/share/utilities/sizes.hpp 

Consider adding
constexpr ByteSize in_ByteSize(WordSize size) { ... }
constexpr int in_bytes(WordSize size) { ... }

Also consider adding the identity conversion
constexpr ByteSize in_ByteSize(ByteSize size) { return size; }
which might be useful in generic code?

But if we're really planning to move away from using WordSize at all, then
probably don't bother.

------------------------------------------------------------------------------
src/hotspot/share/utilities/sizes.hpp

It would be nice to have a more complete set of operations for ByteSize.
That way, when writing new code, one doesn't need to remember or look up
whether the operation you want to use is supported.

ByteSize operator*(int, ByteSize)  -- so multiplicative scaling is commutative

ByteSize operator/(ByteSize, int)  -- division scaling ?

-- op= variants
ByteSize& operator+=(ByteSize& x, ByteSize y) 
ByteSize& operator-=(ByteSize& x, ByteSize y) 
ByteSize& operator*=(ByteSize& x, int y)
ByteSize& operator/=(ByteSize& x, int y)

------------------------------------------------------------------------------
src/hotspot/share/utilities/sizes.hpp 
45 inline WordSize in_WordSize(int size) { return WordSize(size); }
46 inline int      in_words(WordSize x)  { return static_cast<int>(x); }
...
50 inline ByteSize in_ByteSize(int size) { return ByteSize(size); }
51 inline int      in_bytes(ByteSize x)  { return static_cast<int>(x); }

The explicit conversions using function notation are semantically equivalent
to static casts.  I think these should be consistent about which form they
use.  My preference would be for static_cast for all of these.

------------------------------------------------------------------------------
src/hotspot/cpu/arm/templateInterpreterGenerator_arm.cpp
 591     __ add(Rbcp, Rtemp, in_bytes(ConstMethod::codes_offset())); // get codebase

Newly added in_bytes seems like it should not be needed here. codes_offset
returns a ByteSize, and AsmOperand has an implicit ByteSize conversion
constructor.

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