[MacAddress]: Optimize to_string() and parseMacString methods by pavel-shirshov · Pull Request #171 · sonic-net/sonic-swss-common

pavel-shirshov · 2018-01-03T23:04:52Z

MacAddress class instantiated many times from string. Also we parse mac addresses a lot. I did a benchmark and I found that our old version of to_string() and parseMacString() methods not so fast.

I've rewritten both of them. Now both methods faster (parseMacString - 76 times faster, almost 10 times faster) and parseMacString() catch more errors.

2018-01-03 23:01:53
Run on (16 X 2394.44 MHz CPU s)
CPU Caches:
  L1 Data 32K (x16)
  L1 Instruction 32K (x16)
  L2 Unified 256K (x16)
  L3 Unified 30720K (x16)
------------------------------------------------------------
Benchmark                     Time           CPU Iterations
------------------------------------------------------------
BM_ParseMacOld              840 ns        840 ns     842880
BM_ParseMac                  11 ns         11 ns   62703966
BM_MAC_to_string_old        496 ns        496 ns    1448299
BM_MAC_to_string             50 ns         50 ns   13564922

qiluo-msft · 2018-01-03T23:27:59Z

common/macaddress.cpp


-    for (i = 0; i < 5; i++)
+    if ((ptr_mac[2] != ':' || ptr_mac[5] != ':' || ptr_mac[8] != ':' || ptr_mac[11] != ':' || ptr_mac[14] != ':')
+     && (ptr_mac[2] != '-' || ptr_mac[5] != '-' || ptr_mac[8] != '-' || ptr_mac[11] != '-' || ptr_mac[14] != '-'))


Hard to read. #Closed

I don't know how I can make it more readable. Add const del_pos_1 = 2, del_pos_2 = 5 and so on? I doubt it is more readable then now.
What would you suggest?

// ref: [https://stackoverflow.com/a/8198279/2514803](https://stackoverflow.com/a/8198279/2514803) template <typename T, typename U> bool allequal(const T &t, const U &u) { return t == u; } template <typename T, typename U, typename... Others> bool allequal(const T &t, const U &u, Others const &... args) { return (t == u) && allequal(u, args...); } if (!allequal(ptr_mac[2], ptr_mac[5], ptr_mac[8], ptr_mac[11], ptr_mac[14])) return false; if (!(ptr_mac[2] == ':' || ptr_mac[2] == '-')) return false;

In reply to: 159558946 [](ancestors = 159558946)

For me this code even harder to read. In my code only numbers are cryptic, here we have the same numbers, but also some additional operation which adds the complexity.

Even you don't use template version of allequal, you could do hard-coded allequal once, and reduce the number of comparison 10 -> 6.
#Closed

I've added the code you provided. The benchmark doesn't show differences between your and my version. I've added comments to make the code clearer.

qiluo-msft · 2018-01-03T23:28:25Z

common/macaddress.cpp

+    }

-    if (mac == NULL)
+    if (str_mac.length() != 17)


17 [](start = 28, length = 2)

magic number. 6*2 + 5? #Closed

added a constant for the number

qiluo-msft · 2018-01-03T23:29:14Z

common/macaddress.cpp

+        return false;
+    }
+
+    for(int i = 0; i < 6; i++)


for [](start = 4, length = 3)

reuse strtol? #Closed

The performance of strtol isn't good.

In case strtol read an invalid value it returns 0. So it's impossible to distinct between an error and 0.
Conversion of hexadecimal number from string to a binary easy. So this is a reason I write everything in the function and don't use standard functions.

qiluo-msft · 2018-01-03T23:35:12Z

common/macaddress.cpp

+        char left_half = static_cast<char>(mac[i] >> 4);
+        char right_half = mac[i] & 0x0f;
+
+        if (left_half >= 0 && left_half <= 9)


if [](start = 8, length = 2)

Use a static mapping (char->int) to make it even faster? #Closed

I don't know how an array could help here. Can you please elaborate?

const static char map[] = "0123456789abcdef"; str[left] = map[left_half];

In reply to: 159559465 [](ancestors = 159559465)

Thanks Qi. Now it looks better.
But performance almost the same:

2018-01-04 02:26:25 Run on (16 X 2394.44 MHz CPU s) CPU Caches: L1 Data 32K (x16) L1 Instruction 32K (x16) L2 Unified 256K (x16) L3 Unified 30720K (x16) ------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------ BM_ParseMacOld 874 ns 874 ns 798878 BM_ParseMac 13 ns 13 ns 53531585 BM_MAC_to_string_old 467 ns 467 ns 1498719 BM_MAC_to_string 47 ns 47 ns 14613664 BM_MAC_to_string_map 46 ns 46 ns 15300858

qiluo-msft · 2018-01-04T02:26:58Z

common/macaddress.cpp

 {
    bool suc = MacAddress::parseMacString(macStr, m_mac);
-    if (!suc) throw invalid_argument("macStr");
+    if (!suc) throw invalid_argument("can't parse mac address '" + macStr + "'");


invalid_argument("can't parse mac address '" + macStr + "'") [](start = 20, length = 60)

Actually invalid_argument("macStr")) is good enough #Closed

If you check other classes in "swss-common" you'll see we return more descriptive exceptions there.

Also I think it's better to show the reason of the exception

… split the get_part_number and get_vendor API's (sonic-net#171) This PR adds a stub function definition with details as to how it should be implemented by a Y cable vendor. Description Added a stub function definition with a doc string describing how it should be implemented Cleaning up and split some API's for vendors Motivation and Context Firmware upgrade is a functionality that vendors need to implement, this PR adds the definition and a description of how the implementation of this firmware_upgrade API should be How Has This Been Tested? Will be tested once implemented for cleanup API's opened a python shell and executed API's for correctness Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

pavel-shirshov added 3 commits January 3, 2018 13:45

Update MacAddress tests

0d4f190

Optimized version of MacAddress to_string and parse

35afbe1

Changed formatting

4f60e3c

pavel-shirshov requested a review from qiluo-msft January 3, 2018 23:04

pavel-shirshov self-assigned this Jan 3, 2018

pavel-shirshov added the Enhancement label Jan 3, 2018

Change formatting

9d0b5e0

qiluo-msft reviewed Jan 3, 2018

View reviewed changes

pavel-shirshov added 2 commits January 3, 2018 16:03

Addressed some comments

e170f7e

Use a char table for even faster to_string

254a87b

qiluo-msft reviewed Jan 4, 2018

View reviewed changes

Use template magic to compare multiple values to each other

a82f0b5

qiluo-msft approved these changes Jan 4, 2018

View reviewed changes

pavel-shirshov merged commit cb48511 into sonic-net:master Jan 4, 2018

pavel-shirshov deleted the pavelsh/mac branch January 4, 2018 21:44

This was referenced Jan 4, 2018

parseMacString() has issues #91

Closed

[MacAddress]: Remove code from stackoverflow #172

Merged

Conversation

pavel-shirshov commented Jan 3, 2018

Uh oh!

qiluo-msft Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qiluo-msft Jan 3, 2018 •

edited

Loading

qiluo-msft Jan 4, 2018 •

edited

Loading

qiluo-msft Jan 3, 2018 •

edited

Loading

pavel-shirshov Jan 4, 2018 •

edited

Loading

qiluo-msft Jan 3, 2018 •

edited

Loading

qiluo-msft Jan 3, 2018 •

edited

Loading

qiluo-msft Jan 4, 2018 •

edited

Loading