RFR (smallish?) 8061611: Remove deprecated command line flags

Derek White derek.white at oracle.com
Tue Dec 16 20:12:36 UTC 2014

Hi Bengt,

On 12/16/14 6:50 AM, Bengt Rutisson wrote:
> Hi Derek,
> On 2014-12-15 23:24, Derek White wrote:
>> FYI -
>> New webrev shows splitting regression test into simpler alias and 
>> deprecation tests...
>> http://cr.openjdk.java.net/~drwhite/8061611/webrev.03/
> Thanks for fixing up the tests.
> We don't use bug numbers in the test names. Please just name the tests 
> GCDeprecatedOptions.java and GCAliasOptions.java.

This gets back to whether these tests should be immutable and test 
against a specific bug, or more like unit tests that are modified as 
functionality changes. If the former then there would be a growing 
number of test files that would be 99% identical, and they would need 
unique names. But now I think this is a bad idea for these specific 
tests - they should be modified as the implementation is modified.

I think the misunderstanding here is that the test cases were designed 
to cover both the changes for 8061611 as well as testing the upcoming 
re-implementation of alias and deprecation handling. But people can only 
review the code in front of them, not some code from the future :-)

The goal of the re-implementation is to allow a developer to implement 
an alias option or add a deprecation warning by adding one line of code 
to a table (as opposed to ad-hoc code). The goal of these new tests was 
to similarly allow a developer to add a test for an aliased or 
deprecated option by adding one line of code to a table.

i.e If the implementation is driven by a global table, then the tests 
should be as well. Let me know if you think this is a horrible idea.

So that's the end of explaining code */not/* under review :-)

For the code under review:
> GCDeprecatedOptions:
> I'm not sure I think we need the test to verify that the test works.
>   93     String[][] testTestDeprecated = {{"UseTLAB", "+"}};
>   94     testDeprecated(testTestDeprecated, false); // Test the test. 
> The output should NOT mention "UseTLAB" at all.
> But if you feel better leaving it in, I guess I'm fine with it.

OK. I'm a bit new to writing regular regression tests, so maybe I'm 
getting carried away with the concept :-)
> I don't understand this:
>   81       String realOpt = (deprecated.length == 2) ? deprecated[0] : 
> deprecated[1];
>   82       String match = realOpt;
> deprecatedc.length is always 2, right? And why do we need the 
> intermediate variable realOpt?

OK. That was for future expansion.
> I'm also a little concerned about this comment:
>   78       // Searching precisely for deprecation warnings is too hard 
> at the moment.
>   79       // There is no standard format. For now, just search for 
> the option name in the output,
>   80       // which should only be printed if there was some 
> deprecation warning.
> There are only 5 deprecated options, so it seems like we should be 
> able to do stricter checks.
> What do you think about simply doing what we have done for other 
> deprecated options? Just start a process and check the output:
> http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/file/bdf65c8bc1a9/test/gc/startup_warnings/TestDefaultMaxRAMFraction.java
> I think it would be fine to add one test for each of the newly 
> deprecated options, but I guess it is also possible to check all 5 
> options in one test. Still the approach that 
> TestDefaultMaxRAMFraction.java takes seems much simpler.

When I re-implement deprecation messages there will be a standard format 
for these options which will be easier to test. So this code isn't 
precise, but it's sufficient, and will be precise in the next version. 
If that's OK.
> GCAliasOptions:
> Similarlly to in GCDeprecatedOptions I am not convinced about the self 
> test:
>  105     // test the test:
>  106     String[][] testTestAliases = {{"MarkStackSizeMax", 
> "CMSMarkStackSizeMax", "1032"}};
>  107     testAliases(testTestAliases, false); // MarkStackSizeMax is 
> NOT an alias for CMSMarkStackSizeMax.
> About DiagnosticMXBean. I realize that it will not be possible to 
> verify the "=" vs. ":=" semantics if you use the MXBean instead of 
> parsing the PrintFlagsFinal output. But on the other hand I am not so 
> sure this is an important distinction to test as long at the values 
> are correct. I'm fine with parsing the output if you think this is 
> important. The MXBean approach would be fewer lines of code though.

O, I'll try MXBeans.
> If you go with the PrintFlagsFinal version I think it would be nice to 
> make the ALIAS_OPTIONS contain small objects instead of String[]. Then 
> you could have named getters instead of using alias[0], alias[1] and 
> alias[2] in testAliases(). That would make it easier to understand.

The reason for the String arrays was to better match the upcoming C 
implementation and the Java test code:

      * Some flags are actually aliases for other flags. This is often
    part of the process of
      * deprecating a flag, but not all aliases need to be deprecated.
    typedef struct {
       const char* alias_name;
       const char* real_name;
    } AliasedFlag;

    static AliasedFlag aliased_jvm_flags[] = {
       { "DefaultMaxRAMFraction",    "MaxRAMFraction"} //<- this line
    can be pasted into VMAliasOptions.java & VMDeprecatedoptions.java
    and updated.

So the goal was to make it trivial to add to the test case.

I'll get a new webrev out soon...

  - Derek
>> On 12/15/14 1:04 PM, Derek White wrote:
>>> Thanks Bengt - comments below...
>>> On 12/15/14 7:51 AM, Bengt Rutisson wrote:
>>>> Hi Derek,
>>>> On 2014-12-12 15:35, Derek White wrote:
>>>>> This is a request for final re-review.
>>>>> Updated for comments, and added regression test for arguments. 
>>>>> This test is a bit overkill for this bug, but it's extensible for 
>>>>> other deprecated and aliased options.
>>>>> Re-merged with tree.
>>>>> Webrev: http://cr.openjdk.java.net/~drwhite/8061611/webrev.01/
>>>> The code changes look good to me.
>>>> A few comments about the test.
>>>> I don't think it is worth testing that the removed flags produce 
>>>> error messages. Starting a VM takes some time and doing it 13 times 
>>>> just to check that no one by mistake added the flags back seems 
>>>> like a waste of resources to me. The risk that the flags are 
>>>> suddenly added back is almost zero, right?
>>> OK.
>>>> I am also not convinced that a general and extensible test is the 
>>>> way to go here. I think I would prefer a more specific test for the 
>>>> flags that were deprecated now. For tests I think it is more 
>>>> important that the tests are easily readable and understandable 
>>>> than that code duplication is avoided. Thus, I strongly prefer 
>>>> small but specific tests that clearly communicates what went wrong 
>>>> when they fail and are easy to understand how they failed. So, in 
>>>> this case maybe we should even have two tests? 
>>>> TestDeprecatedMarkStackFlags and TestDeprecatedConcGCThreadsFlags.
>>> OK. I'm trying to calibrate how much testing is appropriate and how 
>>> it should be organized. In fact, the other organizing principle was 
>>> to be more "process oriented" - one (or more) regression tests per 
>>> bug fixed. And regression tests would be essentially immutable - 
>>> never expanded to test new cases. Is that about right?
>>> So to make the tests simpler, but bug specific, how about I break 
>>> them up for one test to test aliases, and one test to test 
>>> deprecation warnings?
>>>> Instead of parsing the PrintFlagsFinal output you could use 
>>>> ManagementFactoryHelper.getDiagnosticMXBean().getVMOption() to get 
>>>> the option values and check if they are set correctly. I think that 
>>>> will reduce the number of lines in the test further.
>>> PrintFlagsFinal has the added benefit of noting that a flag was 
>>> explicitly set vs. has a default value. (":=" vs "="). I first used 
>>> bizzare option values to try to test that, but ":=" is definitive.
>>> BTW, a motivation for the alias tests was thinking ahead to the redo 
>>> of "alias" options that I'm planning. I'll want to test that the new 
>>> alias handling code actually works for the remaining aliased 
>>> options. But that can get it's own test file.
>>> Thanks again!
>>>  - Derek

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20141216/4dc04d11/attachment.htm>

More information about the hotspot-gc-dev mailing list