X Tutup
Skip to content

bpo-32147: Improved perfomance of binascii.unhexlify(). #4586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serhiy-storchaka merged 6 commits into from
Feb 26, 2018
Merged

bpo-32147: Improved perfomance of binascii.unhexlify(). #4586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4215f0a
bpo-32147: Improved perfomance of binascii.unhexlify().
sir-sigurd Nov 27, 2017
4f5e429
Added more tests for binascii.a2b_hex().
sir-sigurd Nov 27, 2017
bf57e77
Improved performance of binascii.a2b_qp().
sir-sigurd Nov 27, 2017
6e35854
Partially reverted previous commit as it doesn't provide expected per…
sir-sigurd Nov 28, 2017
adc9d23
Added NEWS entry.
sir-sigurd Nov 28, 2017
d42cb6d
Added my name to NEWS entry and to ACKS.
sir-sigurd Jan 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Lib/test/test_binascii.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,11 @@ def test_hex(self):
self.assertEqual(s, u)
self.assertRaises(binascii.Error, binascii.a2b_hex, t[:-1])
self.assertRaises(binascii.Error, binascii.a2b_hex, t[:-1] + b'q')
self.assertRaises(binascii.Error, binascii.a2b_hex, bytes([255, 255]))
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests for b'0G', b'G0', b'0g' and b'g0'.

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's on to you. Since your initial code shared the table with binascii.a2b_qp I thought that you perhaps want to keep the relation between binascii.a2b_hex and binascii.a2b_qp.

It is better to not squish intermediate commits. Squishing them makes reviewing harder.

Copy link
Copy Markdown
Contributor Author

@sir-sigurd sir-sigurd Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serhiy-storchaka, sorry I removed my previous comment. I'll modify PR to use _PyLong_DigitValue in binascii.a2b_qp a bit later.

self.assertRaises(binascii.Error, binascii.a2b_hex, b'0G')
self.assertRaises(binascii.Error, binascii.a2b_hex, b'0g')
self.assertRaises(binascii.Error, binascii.a2b_hex, b'G0')
self.assertRaises(binascii.Error, binascii.a2b_hex, b'g0')

# Confirm that b2a_hex == hexlify and a2b_hex == unhexlify
self.assertEqual(binascii.hexlify(self.type2test(s)), t)
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ Michael Farrell
Troy J. Farrell
Jim Fasarakis-Hilliard
Mark Favas
Sergey Fedoseev
Boris Feld
Thomas Fenzl
Niels Ferguson
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2017-11-28-10-23-13.bpo-32147.PI2k1Y.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:func:`binascii.unhexlify` is now up to 2 times faster.
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "Patch by your name." Add your name in Misc/ACKS.

Patch by Sergey Fedoseev.
38 changes: 5 additions & 33 deletions Modules/binascii.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,21 +1130,6 @@ binascii_hexlify_impl(PyObject *module, Py_buffer *data)
return _Py_strhex_bytes((const char *)data->buf, data->len);
}

static int
to_int(int c)
{
if (Py_ISDIGIT(c))
return c - '0';
else {
if (Py_ISUPPER(c))
c = Py_TOLOWER(c);
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
}
return -1;
}


/*[clinic input]
binascii.a2b_hex

Expand Down Expand Up @@ -1187,9 +1172,9 @@ binascii_a2b_hex_impl(PyObject *module, Py_buffer *hexstr)
retbuf = PyBytes_AS_STRING(retval);

for (i=j=0; i < arglen; i += 2) {
int top = to_int(Py_CHARMASK(argbuf[i]));
int bot = to_int(Py_CHARMASK(argbuf[i+1]));
if (top == -1 || bot == -1) {
unsigned int top = _PyLong_DigitValue[Py_CHARMASK(argbuf[i])];
unsigned int bot = _PyLong_DigitValue[Py_CHARMASK(argbuf[i+1])];
if (top >= 16 || bot >= 16) {
PyErr_SetString(Error,
"Non-hexadecimal digit found");
goto finally;
Expand Down Expand Up @@ -1218,19 +1203,6 @@ binascii_unhexlify_impl(PyObject *module, Py_buffer *hexstr)
return binascii_a2b_hex_impl(module, hexstr);
}

static const int table_hex[128] = {
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9,-1,-1, -1,-1,-1,-1,
-1,10,11,12, 13,14,15,-1, -1,-1,-1,-1, -1,-1,-1,-1,
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
-1,10,11,12, 13,14,15,-1, -1,-1,-1,-1, -1,-1,-1,-1,
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1
};

#define hexval(c) table_hex[(unsigned int)(c)]

#define MAXLINESIZE 76


Expand Down Expand Up @@ -1293,9 +1265,9 @@ binascii_a2b_qp_impl(PyObject *module, Py_buffer *data, int header)
(ascii_data[in+1] >= 'a' && ascii_data[in+1] <= 'f') ||
(ascii_data[in+1] >= '0' && ascii_data[in+1] <= '9'))) {
/* hexval */
ch = hexval(ascii_data[in]) << 4;
ch = _PyLong_DigitValue[ascii_data[in]] << 4;
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if replace the above checks with _PyLong_DigitValue[ascii_data[in]] < 16 && _PyLong_DigitValue[ascii_data[in+1]] < 16 (with saving intermediate results in variables)?

Copy link
Copy Markdown
Contributor Author

@sir-sigurd sir-sigurd Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean like here? bf57e77#diff-37791d6fab7c332f1ef6b333d28b7aceR1261

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. What were the problems with this?

Copy link
Copy Markdown
Contributor Author

@sir-sigurd sir-sigurd Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It becomes slower than it was for some reason.
Before:

$ ./python -m timeit -s "from binascii import a2b_qp; b = b'=AF'*2**20" "a2b_qp(b)"
100 loops, best of 5: 3.39 msec per loop

After:

$ ./python -m timeit -s "from binascii import a2b_qp; b = b'=AF'*2**20" "a2b_qp(b)"
100 loops, best of 5: 3.72 msec per loop

in++;
ch |= hexval(ascii_data[in]);
ch |= _PyLong_DigitValue[ascii_data[in]];
in++;
odata[out++] = ch;
}
Expand Down
X Tutup