RFR: 8243208: Clean up JVMFlag implementation [v3]

[David Holmes]

Hi Ioi,

On 10/09/2020 1:41 am, Ioi Lam wrote:
> On Wed, 9 Sep 2020 14:06:05 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
> 
>>> src/hotspot/share/runtime/flags/jvmFlag.cpp line 266:
>>>
>>>> 264:
>>>> 265: bool JVMFlag::is_writeable() const {
>>>> 266:   return is_manageable();
>>>
>>> Unclear why is_writeable() reduces to is_manageable(). Aren't all product flags writeable? And aren't all develop flags
>>> writeable in non-product builds?
>>
>> All flags shouldn't be also writeable.  Interpreter code generation depends on static value of some flags, for instance.
> 
> is_writeable() doesn't mean "modifiable". The meaning is
> 
> // manageable flags are writeable external product flags.
> //    They are dynamically writeable through the JDK management interface
> //    (com.sun.management.HotSpotDiagnosticMXBean API) and also through JConsole.
> 
> See also
> [writeableFlags.cpp](https://github.com/openjdk/jdk/blob/433394203dce55db95caefcb57bac9ec114ebc47/src/hotspot/share/services/writeableFlags.cpp#L283).
> 
> There used to be `product_rw` flags, which were "writeable" but not "manageable". However, there aren't any of them
> anymore. So I have deleted product_rw() from the comments.
> /**** removed comments ****/
> // product_rw flags are writeable internal product flags.
> //    They are like "manageable" flags but for internal/private use.
> //    The list of product_rw flags are internal/private flags which
> //    may be changed/removed in a future release.  It can be set
> //    through the management interface to get/set value
> //    when the name of flag is supplied.
> //
> //    A flag can be made as "product_rw" only if
> //    - the VM implementation supports dynamic setting of the flag.
> //      This implies that the VM must *always* query the flag variable
> //      and not reuse state related to the flag state at any given time.
> //
> // Note that when there is a need to support develop flags to be writeable,
> // it can be done in the same way as product_rw.
> 
> I also removed the implementation of product_rw(). Line 269 is part of the product_rw() removal.
> 
> I considered adding support for product_rw() in the new flags implementation. It would be fairly trivial (similar to
> how MANAGEABLE is supported) but I am hesitant to do it because:
> 
> - No one uses it currently
> - There are no test cases
> 
> I don't want to have an untested implementation which someone might accidently use in the future without any testing.
> If anyone indeed finds producy_rw useful, they can easily implement the functionality and can write tests for it at the
> same time.

Yes sorry for the noise. Obviously most flags are not writeable - their 
value has to be fixed at startup. Doh! I was confused by your earlier 
webrev which showed "|| is_product()" and which seemed "obviously" 
correct. :)

I'm somewhat surprised that none of our product or develop flags are 
writeable any more. It used to be some Print/Trace flags that were 
writeable and they have been replaced by UL. I can imagine wanting to 
dynamically enable/disable diagnostic flags though ...

I wonder if "writeable" should be an attribute like 
diagnostic/experimental, so that manageable flags are just writeable 
product flags? That would be a future RFE of course (if appropriate).

Thanks,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/82
>