D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[David Korth]

dkorth added a comment.


  The original version I wrote a few months back as a quick fix to reduce CPU 
usage. I do agree that the magic numbers shouldn't be hard-coded, since that's 
not easily maintained, though I'm not sure of the best approach. constexpr 
strlen() seems like it'd work, or alternatively, splitting the line on spaces.
  
  I'll look into rewriting it to be more robust and submit an update.

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Jiří Paleček]

jpalecek added a comment.


  In D29808#673157 , @sandsmark 
wrote:
  
  > > It's all C code whereas the rest of the helper is C++. It also relies 
very heavily on magic numbers now.
  >
  >
  >
  > > I think a much simpler implementation would be to split each line on " ", 
select the fields we want and clean them up.
  >
  > I assume this is for performance reasons, but a tiny microbenchmark showing 
that it is actually faster would be nice.
  
  
  Hi! Just out of curiosity (I'm not the OP), I created a microbenchmark here: 
https://github.com/jpalecek/ksysguard-network-microbenchmark. As a fun project, 
I added a Boost.Sphinx implementation, which was remarkably easy. Not the most 
readable code ever, beware. Some bugs were found while doing this (in both old 
and new implementation), see the code.

INLINE COMMENTS

> sandsmark wrote in ConnectionMapping.cpp:167
> instead of 87 + 1, maybe have a `constexpr int lineLength = strlen("0: 
> 0100:0277 : 
> 0A : 00:  00 16741");` above with 
> the comment?
> or:
> 
>   constexpr const char *exampleLine = "0: 
> 0100:0277 : 
> 0A : 00:  00 16741";
>   constexpr int lineLength = strlen(exampleLine);
> 
> both more understandable and less prone to accidentally putting in the wrong 
> number.
> 
> same with all other magic string offset numbers here.

Actually I don't agree with that view. I don't think `lineLength = strlen("0: 
000...`)` is readable and less error prone than `87`, only 
longer by ~100 characters and a nightmare to check. With numbers, I can check 
with an editor that shows column numbers and simple arithmetic, but comparing 
such long strings made of spaces is hard.

If you want more robustness, I would suggest using the fact that the number 
columns are aligned with the header and using something like `inode_col = 
header.find("inode")`. Or counting the fields. That would also take care of the 
possibility that the columns may not be same width across architectures (?)

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Martin Tobias Holmedahl Sandsmark]

sandsmark added inline comments.

INLINE COMMENTS

> sandsmark wrote in ConnectionMapping.cpp:167
> instead of 87 + 1, maybe have a `constexpr int lineLength = strlen("0: 
> 0100:0277 : 
> 0A : 00:  00 16741");` above with 
> the comment?
> or:
> 
>   constexpr const char *exampleLine = "0: 
> 0100:0277 : 
> 0A : 00:  00 16741";
>   constexpr int lineLength = strlen(exampleLine);
> 
> both more understandable and less prone to accidentally putting in the wrong 
> number.
> 
> same with all other magic string offset numbers here.

To see that it is actually optimized properly: https://godbolt.org/z/sL2_pK

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Martin Tobias Holmedahl Sandsmark]

sandsmark added inline comments.

INLINE COMMENTS

> ConnectionMapping.cpp:157
> +// Should be within the first 16 characters.
> +size_t data_start = data.find(':');
> +if (data_start >= 16) {

if we're going for ultra optimized, const everything I guess.

> ConnectionMapping.cpp:167
> +// Check for IPv4.
> +if (data.size() < data_start + 87 + 1) {
> +continue;

instead of 87 + 1, maybe have a `constexpr int lineLength = strlen("0: 
0100:0277 : 0A 
: 00:  00 16741");` above with the 
comment?
or:

  constexpr const char *exampleLine = "0: 0100:0277 
: 0A : 00:  
00 16741";
  constexpr int lineLength = strlen(exampleLine);

both more understandable and less prone to accidentally putting in the wrong 
number.

same with all other magic string offset numbers here.

> ConnectionMapping.cpp:176
> +char *const ipv4 = [data_start + 2];
> +ipv4[8] = '\0';
> +localAddress.address[3] = (uint32_t)strtoul(ipv4, nullptr, 16);

strtoul stops at the first non-digit, so is this really necessary?

> ConnectionMapping.cpp:203
> +ipv6[32] = '\0';
> +localAddress.address[3] = (uint32_t)strtoul([24], 
> nullptr, 16);
> +} else {

I prefer the more c++-ish cast of `uint32_t(foo);` (I think I got that from the 
Qt code style guidelines).

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  > It's all C code whereas the rest of the helper is C++. It also relies very 
heavily on magic numbers now.
  
  
  
  > I think a much simpler implementation would be to split each line on " ", 
select the fields we want and clean them up.
  
  I assume this is for performance reasons, but a tiny microbenchmark showing 
that it is actually faster would be nice.

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Arjen Hiemstra]

ahiemstra requested changes to this revision.
ahiemstra added a comment.
This revision now requires changes to proceed.


  Hmm, this is good idea, I'm not really happy about the implementation though. 
It's all C code whereas the rest of the helper is C++. It also relies very 
heavily on magic numbers now.
  
  I think a much simpler implementation would be to split each line on " ", 
select the fields we want and clean them up.

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

To: dkorth, ahiemstra
Cc: ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[David Korth]

dkorth created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dkorth requested review of this revision.

REVISION SUMMARY
  std::regex is slow, so use fixed-field parsing instead.
  
  This is an initial version that I've been using for a while with no major 
issues, though it might still need some improvements. CPU usage is 
significantly decreased compared to before when there is a lot of network 
activity.

TEST PLAN
  Run programs that generate a lot of network activity.
  Verify the network activity shows up correctly with this patch.

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

AFFECTED FILES
  plugins/process/network/helper/Accumulator.cpp
  plugins/process/network/helper/ConnectionMapping.cpp
  plugins/process/network/helper/ConnectionMapping.h

To: dkorth
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart