RFR: 8262328: Templatize JVMFlag boilerplate access methods

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8262328: Templatize JVMFlag boilerplate access methods

Ioi Lam-2
We have a bunch of boilerplate method like:

JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
...

Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}

We should replace such patterns with

template <typename T>
JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)

This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.

The flag access code in whitebox.cpp can also be improved.

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

Commit messages:
 - added test case
 - removed JVMFlag::set_##type() functions
 - fixed merge
 - Merge branch 'master' into 8262328-templatize-jvmflag-boilerplate-methods
 - static_assert to disable SET_FLAG_XXX on ccstr flags
 - more cleanup
 - step2
 - 8262328: Templatize JVMFlag boilerplate methods

Changes: https://git.openjdk.java.net/jdk/pull/3318/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3318&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262328
  Stats: 325 lines in 11 files changed: 71 ins; 135 del; 119 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3318.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3318/head:pull/3318

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods

Ioi Lam-2
On Thu, 1 Apr 2021 22:44:34 GMT, Ioi Lam <[hidden email]> wrote:

> We have a bunch of boilerplate method like:
>
> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
> ...
>
> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>
> We should replace such patterns with
>
> template <typename T>
> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>
> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>
> The flag access code in whitebox.cpp can also be improved.

I need to work on this PR some more. Closing it for now.

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Withdrawn: 8262328: Templatize JVMFlag boilerplate access methods

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 1 Apr 2021 22:44:34 GMT, Ioi Lam <[hidden email]> wrote:

> We have a bunch of boilerplate method like:
>
> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
> ...
>
> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>
> We should replace such patterns with
>
> template <typename T>
> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>
> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>
> The flag access code in whitebox.cpp can also be improved.

This pull request has been closed without being integrated.

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> We have a bunch of boilerplate method like:
>
> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
> ...
>
> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>
> We should replace such patterns with
>
> template <typename T>
> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>
> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>
> The flag access code in whitebox.cpp can also be improved.

Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:

 - removed unnecessary #include
 - Restored <JVM_FLAG_TYPE(type)> in templates so we can require exact types (i.e., cannot mix size_t and uintx even they might be the same type of some platforms)

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3318/files
  - new: https://git.openjdk.java.net/jdk/pull/3318/files/86ce3ce7..58160ff0

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

  Stats: 123 lines in 9 files changed: 32 ins; 17 del; 74 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3318.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3318/head:pull/3318

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Fri, 2 Apr 2021 03:20:51 GMT, Ioi Lam <[hidden email]> wrote:

>> We have a bunch of boilerplate method like:
>>
>> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
>> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
>> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
>> ...
>>
>> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>>
>> We should replace such patterns with
>>
>> template <typename T>
>> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>>
>> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>>
>> The flag access code in whitebox.cpp can also be improved.
>
> I need to work on this PR some more. Closing it for now.

I've updated the patch and the JVMFlagAccess API should have identical behavior as before. Now it's ready to be reviewed again.

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v2]

David Holmes-2
In reply to this post by Ioi Lam-2
On Mon, 5 Apr 2021 17:42:49 GMT, Ioi Lam <[hidden email]> wrote:

>> We have a bunch of boilerplate method like:
>>
>> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
>> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
>> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
>> ...
>>
>> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>>
>> We should replace such patterns with
>>
>> template <typename T>
>> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>>
>> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>>
>> The flag access code in whitebox.cpp can also be improved.
>
> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>
>  - removed unnecessary #include
>  - Restored <JVM_FLAG_TYPE(type)> in templates so we can require exact types (i.e., cannot mix size_t and uintx even they might be the same type of some platforms)

Hi Ioi,

In terms of readability and writeability I prefer <type>AtPut() over set<JVM_FLAG_TYPE(type)>(). Is there any way to push the JVM_FLAG_TYPE down a level so it doesn't shout at you when reading the code? set<type>() would look much much better.

Thanks,
David

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v3]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> We have a bunch of boilerplate method like:
>
> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
> ...
>
> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>
> We should replace such patterns with
>
> template <typename T>
> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>
> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>
> The flag access code in whitebox.cpp can also be improved.

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

  reinstated JVMFlagAccess::set_{bool,int,uint,...} functions for better readability

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3318/files
  - new: https://git.openjdk.java.net/jdk/pull/3318/files/58160ff0..4c0c8ae2

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

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v2]

Ioi Lam-2
In reply to this post by David Holmes-2
On Tue, 6 Apr 2021 02:20:54 GMT, David Holmes <[hidden email]> wrote:

> Hi Ioi,
>
> In terms of readability and writeability I prefer <type>AtPut() over set<JVM_FLAG_TYPE(type)>(). Is there any way to push the JVM_FLAG_TYPE down a level so it doesn't shout at you when reading the code? set<type>() would look much much better.
>
> Thanks,
> David

I added the `JVMFlagAccess::set_<type>()` functions back (they are the same as the old `<type>AtPut` functions, but with a more consistent naming style).

Now I use the more low-level `set<JVM_FLAG_TYPE(type)>()` only in templates where the type is not statically known. E.g, `WriteableFlags::set_flag_impl()`.

* * *

A related note: in an [earlier version](https://webrevs.openjdk.java.net/?repo=jdk&pr=3318&range=00), I tried to get rid of the `<JVM_FLAG_TYPE(type)>` altogether, where the code is much cleaner with template type inference.

JVMFlag* flag = someSizetFlag;
size_t s = 0;
JVMFlagAccess::set(flag, &s, origin);

However, this may allow type mismatch like the following, on platforms that use the exact same type for `size_t` and `uint`:

JVMFlag* flag = someSizetFlag;
uint s = 0;
JVMFlagAccess::set(flag, &s, origin);

but you will get a runtime failure on platforms where `size_t` is not the same as `uint`. So I decided against this design due to non-portability.

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v3]

David Holmes-2
In reply to this post by Ioi Lam-2
On Tue, 6 Apr 2021 03:56:50 GMT, Ioi Lam <[hidden email]> wrote:

>> We have a bunch of boilerplate method like:
>>
>> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
>> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
>> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
>> ...
>>
>> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>>
>> We should replace such patterns with
>>
>> template <typename T>
>> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>>
>> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>>
>> The flag access code in whitebox.cpp can also be improved.
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   reinstated JVMFlagAccess::set_{bool,int,uint,...} functions for better readability

Hi Ioi,

This latest version looks good to me.

Thanks,
David

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v3]

Gerard Ziemski-2
On Thu, 8 Apr 2021 02:04:09 GMT, David Holmes <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   reinstated JVMFlagAccess::set_{bool,int,uint,...} functions for better readability
>
> Hi Ioi,
>
> This latest version looks good to me.
>
> Thanks,
> David

I'm taking a look at your fix Ioi. Building it locally, wanted to check out a few things...

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v3]

Gerard Ziemski-2
In reply to this post by Ioi Lam-2
On Tue, 6 Apr 2021 03:56:50 GMT, Ioi Lam <[hidden email]> wrote:

>> We have a bunch of boilerplate method like:
>>
>> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
>> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
>> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
>> ...
>>
>> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>>
>> We should replace such patterns with
>>
>> template <typename T>
>> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>>
>> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>>
>> The flag access code in whitebox.cpp can also be improved.
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   reinstated JVMFlagAccess::set_{bool,int,uint,...} functions for better readability

Very nice! With all this reduction and "templatization" effort, soon we will be left with one line of a template code at this rate :-)

The only feedback I have is that I wish we could replace `int type_enum` with `FlagType type_enum`

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

Marked as reviewed by gziemski (Committer).

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v3]

Ioi Lam-2
On Fri, 9 Apr 2021 16:17:53 GMT, Gerard Ziemski <[hidden email]> wrote:

> Very nice! With all this reduction and "templatization" effort, soon we will be left with one line of a template code at this rate :-)
>
> The only feedback I have is that I wish we could replace `int type_enum` with `FlagType type_enum`

Hi Gerard, thanks for the review. Replacing the int with enum will cause quite a bit of changes, so I would probably do that in a separate RFE.

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v4]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> We have a bunch of boilerplate method like:
>
> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
> ...
>
> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>
> We should replace such patterns with
>
> template <typename T>
> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>
> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>
> The flag access code in whitebox.cpp can also be improved.

Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:

 - Merge branch 'master' into 8262328-templatize-jvmflag-boilerplate-methods
 - reinstated JVMFlagAccess::set_{bool,int,uint,...} functions for better readability
 - removed unnecessary #include
 - Restored <JVM_FLAG_TYPE(type)> in templates so we can require exact types (i.e., cannot mix size_t and uintx even they might be the same type of some platforms)
 - added test case
 - removed JVMFlag::set_##type() functions
 - fixed merge
 - Merge branch 'master' into 8262328-templatize-jvmflag-boilerplate-methods
 - static_assert to disable SET_FLAG_XXX on ccstr flags
 - more cleanup
 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/c15680e7...e3280b69

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

Changes: https://git.openjdk.java.net/jdk/pull/3318/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3318&range=03
  Stats: 337 lines in 10 files changed: 100 ins; 140 del; 97 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3318.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3318/head:pull/3318

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262328: Templatize JVMFlag boilerplate access methods [v3]

Ioi Lam-2
In reply to this post by David Holmes-2
On Thu, 8 Apr 2021 02:04:09 GMT, David Holmes <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   reinstated JVMFlagAccess::set_{bool,int,uint,...} functions for better readability
>
> Hi Ioi,
>
> This latest version looks good to me.
>
> Thanks,
> David

Thanks @dholmes-ora and @gerard-ziemski for the review!

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

PR: https://git.openjdk.java.net/jdk/pull/3318
Reply | Threaded
Open this post in threaded view
|

Integrated: 8262328: Templatize JVMFlag boilerplate access methods

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 1 Apr 2021 22:44:34 GMT, Ioi Lam <[hidden email]> wrote:

> We have a bunch of boilerplate method like:
>
> JVMFlagAccess::boolAtPut (JVMFlag* f, bool* v, JVMFlagOrigin origin)
> JVMFlagAccess::intAtPut (JVMFlag* f, int* v, JVMFlagOrigin origin)
> JVMFlagAccess::uintAtPut (JVMFlag* f, uint* v, JVMFlagOrigin origin)
> ...
>
> Similarly, we also have 8 different functions: JVMFlag::{set_bool, set_int, set_intx, ...}
>
> We should replace such patterns with
>
> template <typename T>
> JVMFlagAccess::set(JVMFlag* f, T* value, JVMFlagOrigin origin)
>
> This would allow us to templatize the 8x boilerplate functions in writeableFlags.cpp.
>
> The flag access code in whitebox.cpp can also be improved.

This pull request has now been integrated.

Changeset: 627ad9fe
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/627ad9fe
Stats:     337 lines in 10 files changed: 100 ins; 140 del; 97 mod

8262328: Templatize JVMFlag boilerplate access methods

Reviewed-by: dholmes, gziemski

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

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