RFR: 8240222: [TESTBUG] gtest/jfr/test_networkUtilization.cpp failed when the number of tests is greater than or equal to 2

[Jia Huang]

Hi Markus,

Is it a trivial patch and OK to be pushed?
Or shall we wait for one more review?

Best regards,
Jia

在 2020/3/10 19:37, Jia Huang 写道:
> Thanks Markus.
>
> Best regards,
> Jia
>
> 在 2020/3/10 18:15, Markus Gronlund 写道:
>> Hi Jia,
>>
>> Looks like it could work.
>>
>> Thanks
>> Markus
>>
>> -----Original Message-----
>> From: Jia Huang <huangjia at loongson.cn>
>> Sent: den 10 mars 2020 02:30
>> To: Markus Gronlund <markus.gronlund at oracle.com>
>> Cc: hotspot-jfr-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
>> Subject: Re: RFR: 8240222: [TESTBUG] 
>> gtest/jfr/test_networkUtilization.cpp failed when the number of tests 
>> is greater than or equal to 2
>>
>> Thanks Markus.
>>
>> Could you give some comments or advice on this one:
>> Webrev:  http://cr.openjdk.java.net/~jiahuang/8240222/webrev.00/ ?
>>
>> Best regards,
>> Jia
>>
>> 在 2020/3/9 19:41, Markus Gronlund 写道:
>>> The interface_id assigns a unique primary key for each interface.
>>>
>>> To support GTEST=REPEAT, I think the easiest way is to add a 
>>> corresponding counter in the gtest itself that mirror the production 
>>> code:
>>>
>>> That is, as an example, instead of the "add_interface("eth0", 1), that
>>> hardcodes the traceid = 1, have it take a value from the corresponding
>>> counter, i.e. "add_interface("eth0", next_id++)";
>>>
>>> Thanks
>>> Markus
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: Jia Huang <huangjia at loongson.cn>
>>> Sent: den 9 mars 2020 10:57
>>> To: hotspot-dev at openjdk.java.net; hotspot-jfr-dev at openjdk.java.net
>>> Subject: Re: RFR: 8240222: [TESTBUG]
>>> gtest/jfr/test_networkUtilization.cpp failed when the number of tests
>>> is greater than or equal to 2
>>>
>>> In addition, the static local variable "id" is assigned in the 
>>> function of "new_entry", that is in 
>>> share/jfr/periodic/jfrNetworkUtilization.cpp.
>>>
>>> ---------------------------------------------------------------
>>> share/jfr/periodic/jfrNetworkUtilization.cpp
>>>
>>> static InterfaceEntry& new_entry(const NetworkInterface* iface,
>>> GrowableArray<InterfaceEntry>* interfaces) {
>>>      assert(iface != NULL, "invariant");
>>>      assert(interfaces != NULL, "invariant");
>>>
>>>      // single threaded premise
>>>      static traceid interface_id = 0;                       // this is
>>> the static local variable
>>>
>>>      const char* name = iface->get_name();
>>>      assert(name != NULL, "invariant");
>>>
>>>      InterfaceEntry entry;
>>>      const size_t length = strlen(name);
>>>      entry.name = NEW_C_HEAP_ARRAY(char, length + 1, mtInternal);
>>>      strncpy(entry.name, name, length + 1);
>>>      entry.id = ++interface_id;
>>>      entry.bytes_in = iface->get_bytes_in();
>>>      entry.bytes_out = iface->get_bytes_out();
>>>      entry.written = false;
>>>      return _interfaces->at(_interfaces->append(entry));
>>> }
>>> ---------------------------------------------------------------
>>>
>>> 在 2020/3/9 15:29, Jia Huang 写道:
>>>> Haha, I'm also trying to email hotspot-jfr-dev.
>>>> Thank you, David.
>>>>
>>>> 在 2020/3/9 13:03, David Holmes 写道:
>>>>> Hi Jia,
>>>>>
>>>>> I see this is getting no response so have cc'd hotspot-jfr-dev as
>>>>> they should have a better idea of how this gtest can and cannot be 
>>>>> used.
>>>>>
>>>>> Just because the GTest framework supports "REPEAT" it doesn't mean
>>>>> all tests can, or should, be repeatable.
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>> On 29/02/2020 1:53 pm, Jia Huang wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> 在 2020/2/28 21:16, David Holmes 写道:
>>>>>>> Hi Jia,
>>>>>>>
>>>>>>> On 28/02/2020 11:08 pm, Jia Huang wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8240222
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jiahuang/8240222/webrev.00/
>>>>>>>>
>>>>>>>> gtest/jfr/test_networkUtilization.cpp failed when the number of
>>>>>>>> tests is greater than or equal to 2.
>>>>>>>> As shown in the following code, the test failed because "traceid
>>>>>>>> id" is a static local variable.
>>>>>>>> When the test runs a second time, "id" continues to grow,
>>>>>>>> resulting in the return value of "i" being "_interfaces.end()".
>>>>>>> Interesting! I'm not at all familiar with the details of gtest and
>>>>>>> how it actually runs things. But I would expect that we have many
>>>>>>> tests that rely on starting from a cleanly initialized VM state,
>>>>>>> or test state, and so can't simply be repeated (it would be like
>>>>>>> editing an arbitrary test and putting the body of main into a
>>>>>>> for-loop!).
>>>>>> I'm sorry that my last description may be a little incomplete.
>>>>>> JfrTestNetworkUtilization is a group of tests,it includes four
>>>>>> individual tests:
>>>>>> --JfrTestNetworkUtilization.RequestFunctionBasic_test_vm
>>>>>> --JfrTestNetworkUtilization.RequestFunctionMultiple_test_vm
>>>>>> --JfrTestNetworkUtilization.InterfaceRemoved_test_vm
>>>>>>      --JfrTestNetworkUtilization.InterfaceReset_test_vm
>>>>>>
>>>>>> If you only run a individual test once, such as the following
>>>>>> Testing, it still fails.
>>>>>> The reason for the failure, as I said last time, is also because of
>>>>>> static local variables.
>>>>>> In the "RequestFunctionMultiple_test_vm" test, the sequence number
>>>>>> of "eth0" is 2.
>>>>>> However, the initial value of the static local variable "id" is 0.
>>>>>>
>>>>>> Testting:
>>>>>>       --make run-test
>>>>>> TEST=gtest:JfrTestNetworkUtilization.RequestFunctionMultiple_test_v
>>>>>> m
>>>>>> GTEST=REPEAT=1 CONF=release
>>>>>>       --make run-test
>>>>>> TEST=gtest:JfrTestNetworkUtilization.InterfaceRemoved_test_vm
>>>>>> GTEST=REPEAT=1 CONF=release
>>>>>>       --make run-test
>>>>>> TEST=gtest:JfrTestNetworkUtilization.InterfaceReset_test_vm
>>>>>> GTEST=REPEAT=1 CONF=release
>>>>>>
>>>>>>> So I'm not sure what the right way to fix this is:
>>>>>>> - don't use REPEAT?
>>>>>>> - declare the test as not repeatable somehow?
>>>>>>> - change the test to make it repeatable?
>>>>>> In the doc/testing.md or doc/testing.html, Gtest keywords inclue
>>>>>> "REPEAT", so I think this means that all GTEST tests should be able
>>>>>> to use the "REPEAT parameter.
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Jia
>>>>>>
>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>>
>>>>>>>> -----------------------------------------------------------------
>>>>>>>> -----------
>>>>>>>>
>>>>>>>> test/hotspot/gtest/jfr/test_networkUtilization.cpp
>>>>>>>>
>>>>>>>> static const MockNetworkInterface& get_interface(traceid id) {
>>>>>>>> std::list<MockNetworkInterface>::const_iterator i =
>>>>>>>> _interfaces.begin();
>>>>>>>>      for (; i != _interfaces.end(); ++i) {
>>>>>>>>        if (i->id == id) {
>>>>>>>>          break;
>>>>>>>>        }
>>>>>>>>      }
>>>>>>>>      return *i;
>>>>>>>> }
>>>>>>>> -----------------------------------------------------------------
>>>>>>>> -----------
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>>        - make run-test TEST=gtest:JfrTestNetworkUtilization
>>>>>>>> GTEST=REPEAT=2 CONF=server-release
>>>>>>>>        - make run-test TEST=gtest:JfrTestNetworkUtilization
>>>>>>>> GTEST=REPEAT=2 CONF=server-fastdebug
>>>>>>>>
>>>>>>>> Thanks a lot
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Jia
>>>>>>>>