RFR: 8003209: JFR events for network utilization

[Markus Gronlund]

Thanks Thomas and Volker for verifying this so quickly!

We might create follow-ups on the suggestions.

Thanks again
Markus

-----Original Message-----
From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com] 
Sent: den 28 juni 2018 12:51
To: Robin Westberg <robin.westberg at oracle.com>
Cc: hotspot-jfr-dev at openjdk.java.net; Markus Grönlund <markus.gronlund at oracle.com>; Volker Simonis <volker.simonis at gmail.com>
Subject: Re: RFR: 8003209: JFR events for network utilization

Hi Robin,

I tested AIX, it did build fine and did not show any obvious errors.
Did not have time yet to test the functionality though.

But the patch is fine as it is from our side.

Some small nits (can of course be done in a follow up):

----

src/hotspot/os/linux/os_perf_linux.cpp

+  if (_impl != NULL) {
+    delete _impl;
+  }

Strictly speaking not necessary, since delete NULL is allowed

--

+int NetworkPerformanceInterface::network_utilization(NetworkInterface**
network_interfaces) const {
+  return _impl->network_utilization(network_interfaces);
+}

Test for impl == NULL? in case initialization failed?

---

+  ssize_t num_bytes = read(fd, buf, sizeof(buf));  close(fd);
...
+
+  buf[num_bytes] = '\0';

theoretical overwriter here if the file happens to be longer than 128
- I suggest:

- read(fd, buf, sizeof(buf));
+ read(fd, buf, sizeof(buf) - 1);


--

+  int64_t value = strtoll(buf, NULL, 10);

I'd probably check for ERANGE/EINVAL errors here before returning
LLONG_MIN or LLONG_MAX.

..

Thanks, Thomas



On Tue, Jun 26, 2018 at 9:41 PM, Robin Westberg
<robin.westberg at oracle.com> wrote:
> Hi yet one more time,
>
> Markus found another optimization that allows us to store timestamps once instead of per interface, which should save a bit of space.
>
> Incremental: http://cr.openjdk.java.net/~rwestberg/8003209/webrev.02-03/ <http://cr.openjdk.java.net/~rwestberg/8003209/webrev.02-03/>
> Full: http://cr.openjdk.java.net/~rwestberg/8003209/webrev.03/ <http://cr.openjdk.java.net/~rwestberg/8003209/webrev.03/>
>
> Best regards,
> Robin
>
>> On 26 Jun 2018, at 21:14, Robin Westberg <robin.westberg at oracle.com> wrote:
>>
>> Hi once again,
>>
>> After some additional off-list feedback from Markus I’ve also added a stub implementation for aix (that hopefully compiles), as well as cleaned up the headers included in the unit test.
>>
>> Incremental: http://cr.openjdk.java.net/~rwestberg/8003209/webrev.01-02/ <http://cr.openjdk.java.net/~rwestberg/8003209/webrev.01-02/>
>> Full: http://cr.openjdk.java.net/~rwestberg/8003209/webrev.02/ <http://cr.openjdk.java.net/~rwestberg/8003209/webrev.02/>
>>
>> Best regards,
>> Robin
>>
>>> On 26 Jun 2018, at 15:01, Robin Westberg <robin.westberg at oracle.com> wrote:
>>>
>>> Hi again,
>>>
>>> Markus kindly provided some off-list feedback with a patch to make the serialization of interfaces names more efficient. I’ve incorporated those changes, as well as adapted the unit test and corrected a few copyright years.
>>>
>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8003209/webrev.00-01/ <http://cr.openjdk.java.net/~rwestberg/8003209/webrev.00-01/>
>>> Full: http://cr.openjdk.java.net/~rwestberg/8003209/webrev.01/ <http://cr.openjdk.java.net/~rwestberg/8003209/webrev.01/>
>>>
>>> Best regards,
>>> Robin
>>>
>>>> On 25 Jun 2018, at 21:04, Robin Westberg <robin.westberg at oracle.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Please review the following change with adds a JFR event for tracking network utilization on the network interface level.
>>>>
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8003209 <https://bugs.openjdk.java.net/browse/JDK-8003209>
>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8003209/webrev.00/ <http://cr.openjdk.java.net/~rwestberg/8003209/webrev.00/>
>>>> Testing: test/jdk/jdk/jfr/*
>>>>
>>>> Many thanks to Erik Gahlin and Markus Grönlund who have looked at earlier versions of this change and suggested valuable improvements.
>>>>
>>>> Best regards,
>>>> Robin
>>>
>>
>