OpenJDK › OpenJDK Hotspot › OpenJDK Hotspot Runtime System

RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp

Classic

List

Threaded

15 messages Options

Ioi Lam-2

RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp

We have the following in Arguments::parse_xss():

const julong min_ThreadStackSize = 0;
const julong max_ThreadStackSize = 1 * M;
const julong min_size = min_ThreadStackSize * K;
const julong max_size = max_ThreadStackSize * K;

which duplicates the min/max range specified in globals.hpp:

product_pd(intx, ThreadStackSize, \
"Thread Stack Size (in Kbytes)") \
range(0, 1 * M) \

I added an API to query the min/max range of a flag, so parse_xss can be rewritten as

const JVMTypedFlagLimit<intx>* limit =
JVMFlagLimit::get_range_at(FLAG_MEMBER_ENUM(ThreadStackSize))->cast<intx>();
const julong min_size = limit->min() * K;
const julong max_size = limit->max() * K;

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

Commit messages:
- fixed typo
- use type_traits to avoid lots of boiler-plate functions
- step one

Changes: https://git.openjdk.java.net/jdk/pull/2688/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8183569
Stats: 79 lines in 4 files changed: 59 ins; 2 del; 18 mod
Patch: https://git.openjdk.java.net/jdk/pull/2688.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2688/head:pull/2688

PR: https://git.openjdk.java.net/jdk/pull/2688

Thomas Stuefe

Re: RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp

On Tue, 23 Feb 2021 07:55:38 GMT, Ioi Lam <[hidden email]> wrote:

> We have the following in Arguments::parse_xss():
>
> const julong min_ThreadStackSize = 0;
> const julong max_ThreadStackSize = 1 * M;
> const julong min_size = min_ThreadStackSize * K;
> const julong max_size = max_ThreadStackSize * K;
>
> which duplicates the min/max range specified in globals.hpp:
>
> product_pd(intx, ThreadStackSize, \
> "Thread Stack Size (in Kbytes)") \
> range(0, 1 * M) \
>
> I added an API to query the min/max range of a flag, so parse_xss can be rewritten as
>
> const JVMTypedFlagLimit<intx>* limit =
> JVMFlagLimit::get_range_at(FLAG_MEMBER_ENUM(ThreadStackSize))->cast<intx>();
> const julong min_size = limit->min() * K;
> const julong max_size = limit->max() * K;

Hi Ioi,

Seems reasonable. Some remarks below.

..Thomas

src/hotspot/share/runtime/flags/jvmFlag.hpp line 314:

> 312: #endif
> 313: }
> 314:

Do we need this? Seems an unused duplicate to the version in jvmFlagLimit.hpp.

src/hotspot/share/runtime/flags/jvmFlagLimit.hpp line 155:

> 153: case JVMFlag::TYPE_uintx: assert(sizeof(T) == sizeof(uintx) && std::is_signed<T>::value == std::is_signed<uintx> ::value, "must be"); break;
> 154: case JVMFlag::TYPE_uint64_t: assert(sizeof(T) == sizeof(uint64_t) && std::is_signed<T>::value == std::is_signed<uint64_t>::value, "must be"); break;
> 155: case JVMFlag::TYPE_size_t: assert(sizeof(T) == sizeof(size_t) && std::is_signed<T>::value == std::is_signed<size_t> ::value, "must be"); break;

Could we shorten the signed comparisons to the expected default? For most types the signedness wont be a surprise :) size_t is unsigned on all our platforms.

is_signed<bool> seems weird, isn't that UB?

src/hotspot/share/runtime/flags/jvmFlagLimit.hpp line 159:

> 157: }
> 158: } else {
> 159: assert(flag_enum == JVMFlag::TYPE_double, "must be double");

Could the same comparisons not be made too here?

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp [v2]

In reply to this post by Ioi Lam-2

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

use macros to simplify assert_compatible_type; restore code in parse_xss so the comments make sense

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2688/files
- new: https://git.openjdk.java.net/jdk/pull/2688/files/0386a37e..8c4b5cd9

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=00-01

Stats: 48 lines in 3 files changed: 13 ins; 30 del; 5 mod
Patch: https://git.openjdk.java.net/jdk/pull/2688.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2688/head:pull/2688

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp [v2]

In reply to this post by Thomas Stuefe

On Tue, 23 Feb 2021 08:10:26 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>> use macros to simplify assert_compatible_type; restore code in parse_xss so the comments make sense
>
> src/hotspot/share/runtime/flags/jvmFlag.hpp line 314:
>
>> 312: #endif
>> 313: }
>> 314:
>
> Do we need this? Seems an unused duplicate to the version in jvmFlagLimit.hpp.

I left `JVMFlagLimit::assert_compatible_type` by mistake, so I deleted it. I think we can use `JVMFlag::assert_compatible_type` in other places as well to get rid of the 8x duplicated boilerplate functions such as `JVMFlag::{get_bool, get_int, get_intx, etc}`. I'll investigate and file an RFE.

For your other comments, I updated `JVMFlag::assert_compatible_type` to use macros, so there's less repetition and more uniform (no more special case for `double`). It's a little easier to maintain, but maybe not as obvious to read.

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

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp [v2]

In reply to this post by Thomas Stuefe

On Tue, 23 Feb 2021 08:18:00 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>> use macros to simplify assert_compatible_type; restore code in parse_xss so the comments make sense
>
> Hi Ioi,
>
> Seems reasonable. Some remarks below.
>
> ..Thomas

In rev [8c4b5cd](https://github.com/openjdk/jdk/commit/8c4b5cd9288b063482f28836ca5b0b337a412d5d):

I updated `Arguments::parse_xss()` to restore the original code -- I think it's important to see the `1 * M` to make sense of what this comment is talking about:

// The min and max sizes match the values in globals.hpp, but scaled
// with K. The values have been chosen so that alignment with page
// size doesn't change the max value, which makes the conversions
// back and forth between Xss value and ThreadStackSize value easier.
// The values have also been chosen to fit inside a 32-bit signed type.
const julong min_ThreadStackSize = 0;
const julong max_ThreadStackSize = 1 * M;

So instead of the PR's initially proposed changes, I added these to make sure that the value `1 * M` matches with globals.hpp:

// Make sure the above values match the range set in globals.hpp
const JVMTypedFlagLimit<intx>* limit = JVMFlagLimit::get_range_at(FLAG_MEMBER_ENUM(ThreadStackSize))->cast<intx>();
assert(min_ThreadStackSize == static_cast<julong>(limit->min()), "must be");
assert(max_ThreadStackSize == static_cast<julong>(limit->max()), "must be");

If people are OK with this, I will update the PR's initial comment, and the bug title as well.

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

PR: https://git.openjdk.java.net/jdk/pull/2688

Kim Barrett-2

Re: RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp [v2]

In reply to this post by Ioi Lam-2

On Tue, 23 Feb 2021 17:31:08 GMT, Ioi Lam <[hidden email]> wrote:

>> We have the following in Arguments::parse_xss():
>>
>> const julong min_ThreadStackSize = 0;
>> const julong max_ThreadStackSize = 1 * M;
>> const julong min_size = min_ThreadStackSize * K;
>> const julong max_size = max_ThreadStackSize * K;
>>
>> which duplicates the min/max range specified in globals.hpp:
>>
>> product_pd(intx, ThreadStackSize, \
>> "Thread Stack Size (in Kbytes)") \
>> range(0, 1 * M) \
>>
>> I added an API to query the min/max range of a flag, so parse_xss can be rewritten as
>>
>> const JVMTypedFlagLimit<intx>* limit =
>> JVMFlagLimit::get_range_at(FLAG_MEMBER_ENUM(ThreadStackSize))->cast<intx>();
>> const julong min_size = limit->min() * K;
>> const julong max_size = limit->max() * K;
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
> use macros to simplify assert_compatible_type; restore code in parse_xss so the comments make sense

Changes requested by kbarrett (Reviewer).

src/hotspot/share/runtime/flags/jvmFlagLimit.hpp line 75:

> 73: #ifndef PRODUCT
> 74: int _type_enum;
> 75: #endif

I think all the `#ifndef PRODUCT` stuff around _type_enum should instead be `#ifdef ASSERT`. These belong in debug builds, but not in optimize builds.

src/hotspot/share/runtime/flags/jvmFlag.hpp line 295:

> 293:
> 294: // type checking
> 295: #define CHECK_COMPATIBLE(type) \

CHECK_COMPATIBLE doesn't appear to be used outside the following assertion function. Add an `#undef` afterward.

src/hotspot/share/runtime/flags/jvmFlag.hpp line 299:

> 297: assert(sizeof(T) == sizeof(type) && \
> 298: std::is_integral<T>::value == std::is_integral<type>::value && \
> 299: std::is_signed <T>::value == std::is_signed <type>::value, "must be"); break;

I'd prefer the `break` be on it's own source line, rather than after the assert form.

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

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Remove duplicated flag limits in parse_xss and globals.hpp [v3]

In reply to this post by Ioi Lam-2

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

@kimbarrett comments

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2688/files
- new: https://git.openjdk.java.net/jdk/pull/2688/files/8c4b5cd9..b99d99ed

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=01-02

Stats: 6 lines in 2 files changed: 3 ins; 0 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/2688.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2688/head:pull/2688

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Assert the same limits are used in parse_xss and globals.hpp [v2]

In reply to this post by Kim Barrett-2

On Mon, 1 Mar 2021 18:10:52 GMT, Kim Barrett <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>> use macros to simplify assert_compatible_type; restore code in parse_xss so the comments make sense
>
> Changes requested by kbarrett (Reviewer).

I've updated the code per @kimbarrett's suggestions, and also changed the title to "Assert the same limits are used in parse_xss and globals.hpp"

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

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Assert the same limits are used in parse_xss and globals.hpp [v4]

In reply to this post by Ioi Lam-2

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

fixed optimized build

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2688/files
- new: https://git.openjdk.java.net/jdk/pull/2688/files/b99d99ed..77ca66c9

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=02-03

Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2688.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2688/head:pull/2688

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Assert the same limits are used in parse_xss and globals.hpp [v5]

In reply to this post by Ioi Lam-2

Ioi Lam has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:

fixed optimized build

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2688/files
- new: https://git.openjdk.java.net/jdk/pull/2688/files/77ca66c9..e28c07cc

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=03-04

Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/2688.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2688/head:pull/2688

PR: https://git.openjdk.java.net/jdk/pull/2688

Kim Barrett-2

Re: RFR: 8183569: Assert the same limits are used in parse_xss and globals.hpp [v5]

On Tue, 2 Mar 2021 01:30:23 GMT, Ioi Lam <[hidden email]> wrote:

>> We have the following in Arguments::parse_xss():
>>
>> const julong min_ThreadStackSize = 0;
>> const julong max_ThreadStackSize = 1 * M;
>> const julong min_size = min_ThreadStackSize * K;
>> const julong max_size = max_ThreadStackSize * K;
>>
>> which duplicates the min/max range specified in globals.hpp:
>>
>> product_pd(intx, ThreadStackSize, \
>> "Thread Stack Size (in Kbytes)") \
>> range(0, 1 * M) \
>>
>> I added an API to query the min/max range of a flag, so parse_xss can be rewritten as
>>
>> const JVMTypedFlagLimit<intx>* limit =
>> JVMFlagLimit::get_range_at(FLAG_MEMBER_ENUM(ThreadStackSize))->cast<intx>();
>> const julong min_size = limit->min() * K;
>> const julong max_size = limit->max() * K;
>
> Ioi Lam has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
>
> fixed optimized build

Looks good.

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

Marked as reviewed by kbarrett (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2688

Thomas Stuefe

Re: RFR: 8183569: Assert the same limits are used in parse_xss and globals.hpp [v5]

In reply to this post by Ioi Lam-2

On Tue, 2 Mar 2021 01:37:08 GMT, Ioi Lam <[hidden email]> wrote:

>> We have the following in Arguments::parse_xss():
>>
>> const julong min_ThreadStackSize = 0;
>> const julong max_ThreadStackSize = 1 * M;
>> const julong min_size = min_ThreadStackSize * K;
>> const julong max_size = max_ThreadStackSize * K;
>>
>> which duplicates the min/max range specified in globals.hpp:
>>
>> product_pd(intx, ThreadStackSize, \
>> "Thread Stack Size (in Kbytes)") \
>> range(0, 1 * M) \
>>
>> I added an API to query the min/max range of a flag, so parse_xss can be rewritten as
>>
>> const JVMTypedFlagLimit<intx>* limit =
>> JVMFlagLimit::get_range_at(FLAG_MEMBER_ENUM(ThreadStackSize))->cast<intx>();
>> const julong min_size = limit->min() * K;
>> const julong max_size = limit->max() * K;
>
> Ioi Lam has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR.

Looks good to me. Sorry for the delay.

Cheers, Thomas

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

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Assert the same limits are used in parse_xss and globals.hpp [v6]

In reply to this post by Ioi Lam-2

Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:

- Merge branch 'master' of https://github.com/openjdk/jdk into 8183569-duplicated-flag-limits-parse-xss
- fixed optimized build
- @kimbarrett comments
- use macros to simplify assert_compatible_type; restore code in parse_xss so the comments make sense
- fixed typo
- use type_traits to avoid lots of boiler-plate functions
- step one

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

Changes:
- all: https://git.openjdk.java.net/jdk/pull/2688/files
- new: https://git.openjdk.java.net/jdk/pull/2688/files/e28c07cc..e0ad72f6

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=05
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2688&range=04-05

Stats: 15113 lines in 689 files changed: 8129 ins; 3848 del; 3136 mod
Patch: https://git.openjdk.java.net/jdk/pull/2688.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2688/head:pull/2688

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Re: RFR: 8183569: Assert the same limits are used in parse_xss and globals.hpp [v5]

In reply to this post by Kim Barrett-2

On Tue, 2 Mar 2021 01:29:27 GMT, Kim Barrett <[hidden email]> wrote:

>> Ioi Lam has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR.
>
> Looks good.

Thanks @kimbarrett and @tstuefe for the review!

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

PR: https://git.openjdk.java.net/jdk/pull/2688

Ioi Lam-2

Integrated: 8183569: Assert the same limits are used in parse_xss and globals.hpp

In reply to this post by Ioi Lam-2

On Tue, 23 Feb 2021 07:55:38 GMT, Ioi Lam <[hidden email]> wrote:

This pull request has now been integrated.

Changeset: 044e2a2a
Author: Ioi Lam <[hidden email]>
URL: https://git.openjdk.java.net/jdk/commit/044e2a2a
Stats: 58 lines in 4 files changed: 43 ins; 0 del; 15 mod

8183569: Assert the same limits are used in parse_xss and globals.hpp

Reviewed-by: stuefe, kbarrett

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

PR: https://git.openjdk.java.net/jdk/pull/2688