Performance improvements for pyre2 #11

fim · 2018-07-30T17:56:10Z

As part of a quick test I did to measure the performance of pyre2, I noticed
hat we spent a lot of time allocating objects and that we could improve
the performance simply by altering some of the C code to limit unnecessary
object allocations. The main culprit was Py_BuildValue but it seems like the
changes had a positive effect overall in terms of performance.

As part of that I did:

Deleted code that populated attributes of the attr_dict struct
Deleted code that initialized the struct itself

This is more of a request for comments about these changes, just to get some initial feedback based on the changes because I might be missing something obvious but as far as I can see nothing has broken and we have performance improvements ranging from 5% to 50% depending on the use case.

Before change:

In [1]: import re2

In [2]: import timeit

In [3]: r = re2.compile('\w+')

In [4]: %timeit r.match('foo bar')
1000000 loops, best of 3: 408 ns per loop

In [5]: r = re2.compile('(?P<test>\w*)')

In [6]: %timeit r.match('foo bar')
1000000 loops, best of 3: 419 ns per loop

In [7]: email_regex = '(?:[a-z0-9!#$%&\'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&\'*+/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9]))\.){3}(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9])|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])'

In [8]: r = re2.compile(email_regex)

In [9]: %timeit r.match('foo@bar.com')
1000000 loops, best of 3: 1.04 µs per loop

After change:

In [1]: import re2

In [2]: import timeit

In [3]: r = re2.compile('\w+')

In [4]: %timeit r.match('foo bar')
1000000 loops, best of 3: 178 ns per loop

In [5]: r = re2.compile('(?P<test>\w*)')

In [6]: %timeit r.match('foo bar')
1000000 loops, best of 3: 184 ns per loop

In [7]: email_regex = '(?:[a-z0-9!#$%&\'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&\'*+/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9]))\.){3}(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9])|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])'

In [8]: r = re2.compile(email_regex)

In [9]: %timeit r.match('foo@bar.com')
1000000 loops, best of 3: 774 ns per loop

PS: Also here's pprof execution graphs for timeit() test before and after the changes:

dreiss · 2018-07-30T18:19:06Z

Doesn't this break compatibility with programs that access m.pos, etc.?

fim · 2018-07-30T23:08:08Z

With the current state of the PR, these are not exposed to the python module mostly because I wasn't sure what's the desired API/ABI that the re2 module should expose.

However, that info is already saved on the MatchObject2 and can be extracted really easy and efficiently with getter methods. Possible patch below:

diff --git a/_re2.cc b/_re2.cc
index 8e3d17d..673a3ba 100644
--- a/_re2.cc
+++ b/_re2.cc
@@ -39,6 +39,8 @@ using std::nothrow;
 using re2::RE2;
 using re2::StringPiece;
 
+#include <structmember.h>
+
 
 typedef struct _RegexpObject2 {
   PyObject_HEAD
@@ -59,6 +61,41 @@ typedef struct _MatchObject2 {
   StringPiece* groups;
 } MatchObject2;
 
+// getters for MatchObject2
+static PyObject* match_pos_get(MatchObject2* self)
+{
+  return PyLong_FromSsize_t(self->pos);
+}
+
+static PyObject* match_endpos_get(MatchObject2* self)
+{
+  return PyLong_FromSsize_t(self->endpos);
+}
+
+static PyObject* match_re_get(MatchObject2* self)
+{
+  if (self->re != NULL)
+    Py_INCREF(self->string);
+    return self->re;
+  Py_RETURN_NONE;
+}
+
+static PyObject* match_string_get(MatchObject2* self)
+{
+  if (self->string != NULL)
+    Py_INCREF(self->string);
+    return self->string;
+  Py_RETURN_NONE;
+}
+
+static PyGetSetDef match_getset[] = {
+    {"pos", (getter)match_pos_get, (setter)NULL},
+    {"endpos", (getter)match_endpos_get, (setter)NULL},
+    {"re",      (getter)match_re_get,      (setter)NULL},
+    {"string",      (getter)match_string_get,      (setter)NULL},
+    {NULL}
+};
+
 typedef struct _RegexpSetObject2 {
   PyObject_HEAD
   // True iff re2_set_obj has been compiled.
@@ -245,7 +282,7 @@ static PyTypeObject Match_Type2 = {
   0,                           /*tp_iternext*/
   match_methods,               /*tp_methods*/
   0,                           /*tp_members*/
-  0,                           /*tp_getset*/
+  match_getset,                /*tp_getset*/
 };
 
 static PyTypeObject RegexpSet_Type2 = {
@@ -475,13 +512,20 @@ create_match(PyObject* re, PyObject* string,
     StringPiece* groups)
 {
   MatchObject2* match = PyObject_New(MatchObject2, &Match_Type2);
+
   if (match == NULL) {
     delete[] groups;
     return NULL;
   }
+  Py_INCREF(groups);
   match->groups = groups;
+  Py_INCREF(re);
   match->re = re;
+  Py_INCREF(string);
   match->string = string;
+  match->pos = pos;
+  match->endpos = endpos;
+
   return (PyObject*)match;
 }

That gives us:

In [1]: import re2

In [2]: r = re2.compile('\w+')

In [3]: m = r.match('foo bar')

In [4]: m.re
Out[4]: <_re2.RE2_Regexp at 0x7f096eb58618>

In [5]: m.string
Out[5]: 'foo bar'

In [6]: m.pos, m.endpos
Out[6]: (0L, 7L)

dreiss · 2018-07-30T23:47:20Z

Cool. Can you update the PR with the full change? Unless @sunshowers has objections, I'm not going to worry about the performance of lazy access since these seem like they'd be rarely accessed.

fim · 2018-07-31T10:04:27Z

Sounds good. In that case, I'll merge the proposed patch to the PR, add some unit tests and do some more thorough testing to make sure I haven't messed up with any object referencing and then push the update.

fim · 2018-08-06T12:49:55Z

Any thoughts on this @sunshowers, @dreiss?

dreiss · 2018-08-07T23:00:50Z

_re2.cc


 typedef struct _RegexpObject2 {
  PyObject_HEAD
-  // __dict__.  Simpler than implementing getattr and possibly faster.
-  PyObject* attr_dict;


Is this part of the change necessary or beneficial? Regexp objects should be constructed very rarely in performance-sensitive programs.

Not sure I got that. The dict stores groups/groupindex and pattern fields. I didn't change the way re2 objects are constructed/destroyed.

However, based on your feedback I'll re-implement these fields for RegexpObject2 (probably using getter/setter to avoid the performance penalties)

Ah, I see what you mean. In general execution, you create the regex once and then just match against it! That's true but I think we can get some performance improvements out of this so, why not :)

I think the default choice should be to leave working code as-is. Let's try to keep this PR scoped to changes that have measurable benefits. If you think changing RegexpObject2 is useful, let's handle that separately.

dreiss · 2018-08-07T23:01:17Z

_re2.cc

  PyObject* re;
  PyObject* string;
+  Py_ssize_t pos, endpos; /* current target slice */


Please put these on separate lines.

dreiss · 2018-08-07T23:02:27Z

_re2.cc

+
+static PyObject* match_re_get(MatchObject2* self)
+{
+  if (self->re != NULL) {


When would this be null?

dreiss · 2018-08-07T23:02:45Z

_re2.cc

+
+static PyObject* match_string_get(MatchObject2* self)
+{
+  if (self->string != NULL) {


When would this be null?

dreiss · 2018-08-07T23:02:58Z

_re2.cc

+  Py_RETURN_NONE;
+}
+
+static PyGetSetDef match_getset[] = {


Formatting looks weird.

dreiss · 2018-08-07T23:05:43Z

_re2.cc

@@ -65,6 +61,43 @@ typedef struct _MatchObject2 {
  StringPiece* groups;
 } MatchObject2;

+// getters for MatchObject2
+static PyObject* match_pos_get(MatchObject2* self)


Why not use the proper getter signature here and just cast to MatchObject2 internally?

Silly question but when you say "proper getter signature", what do you mean?

I had a look at other python extensions and the python documentation before implementing the getters and they both suggest using this kind of signatures. Eg: https://docs.python.org/2.7/extending/newtypes.html

dreiss · 2018-08-07T23:07:14Z

_re2.cc

@@ -216,14 +249,6 @@ static PyTypeObject Regexp_Type2 = {
  regexp_methods,              /*tp_methods*/
  0,                           /*tp_members*/
  0,                           /*tp_getset*/
-  0,                           /*tp_base*/


Why remove these?

By default all omitted entries are 0 and since we're no longer setting the attr_dict I thought i'd trim down the list to be consistent with the other typedef in the source. I can zero the fields out and leave it as is though if that's preferred.

Yeah, I'd leave it. Explicit is better than implicit.

dreiss · 2018-08-07T23:07:25Z

_re2.cc

@@ -259,15 +284,7 @@ static PyTypeObject Match_Type2 = {
  0,                           /*tp_iternext*/
  match_methods,               /*tp_methods*/
  0,                           /*tp_members*/
-  0,                           /*tp_getset*/
-  0,                           /*tp_base*/


Why remove these?

As above, we no longer have the attr_dict so I trimmed the PyType object after the last member we manually set.

Can you leave the zeroes? I think explicit is better than implicit in this case.

dreiss · 2018-08-07T23:09:33Z

_re2.cc

-  // to groupindex.
-  regexp->attr_dict = Py_BuildValue("{sisNsO}",
-      "groups", regexp->re2_obj->NumberOfCapturingGroups(),
-      "groupindex", groupindex,


Does this mean that groupindex no longer works? In case it wasn't clear before, I don't think this change should be breaking any functionality that currently works.

I will address that.

I will do some checks to see if the attr_dict is performant enough otherwise I'll just extend the RegexObject2 definition similiar to MatrchObject2

fim · 2018-08-09T16:18:53Z

Updated the PR and unit tests to cover the re attributes as well.

fim · 2018-10-01T17:47:32Z

@dreiss have you had any time to review my latest changes?

ivan · 2018-10-09T16:17:46Z

I have nothing to do with this project, but I tested your PR:

pyre2 1.0.6:

# ~/re2-venv/bin/python
Python 3.7.0 (default, Sep 28 2018, 07:40:17)
[GCC 6.3.0 20170516] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import re2
>>> re2.compile(")")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/grab/re2-venv/lib/python3.7/site-packages/re2.py", line 56, in compile
    return _compile(pattern, error)
re.error: 6

with this PR applied:

# ~/re2-venv/bin/python
Python 3.7.0 (default, Sep 28 2018, 07:40:17)
[GCC 6.3.0 20170516] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import re2
>>> re2.compile(")")
zsh: segmentation fault (core dumped)  ~/re2-venv/bin/python

I tested with a pyenv-built Python 3.7.0 on an amd64 Debian 9.5 machine.

# dpkg -l | grep re2
ii  libre2-3:amd64                  20170101+dfsg-1                   amd64        efficient, principled regular expression library
ii  libre2-dev                      20170101+dfsg-1                   amd64        efficient, principled regular expression library, development kit

I see the same with a pyenv-built Python 3.7.0 on an amd64 Ubuntu 16.04.5 machine.

# dpkg -l | grep re2
ii  libre2-1v5:amd64                 20160201+dfsg-1                            amd64        efficient, principled regular expression library
ii  libre2-dev                       20160201+dfsg-1                            amd64        efficient, principled regular expression library, development kit

facebook/pyre2#11 (comment)

dreiss

Are you able to repro that crash?

dreiss · 2018-10-09T21:10:09Z

_re2.cc

 typedef struct _RegexpSetObject2 {
  PyObject_HEAD
  // True iff re2_set_obj has been compiled.
  bool compiled;
  RE2::Set* re2_set_obj;
 } RegexpSetObject2;

-


I deleted the extra lines in order to be consistent with spacing but I'll revert all style changes (including the tab -> space change)

dreiss · 2018-10-09T21:10:20Z

_re2.cc

@@ -96,7 +119,6 @@ static PyObject* regexp_set_add(RegexpSetObject2* self, PyObject* pattern);
 static PyObject* regexp_set_compile(RegexpSetObject2* self);
 static PyObject* regexp_set_match(RegexpSetObject2* self, PyObject* text);

-


Same as above

dreiss · 2018-10-09T21:10:32Z

_re2.cc

@@ -170,9 +192,9 @@ static int
 _no_setattr(PyObject* obj, PyObject* name, PyObject* v) {
  (void)name;
  (void)v;
-	PyErr_Format(PyExc_AttributeError,


Should be a separate commit.

Same as bove

dreiss · 2018-10-09T21:10:51Z

_re2.cc

@@ -259,15 +284,7 @@ static PyTypeObject Match_Type2 = {
  0,                           /*tp_iternext*/
  match_methods,               /*tp_methods*/
  0,                           /*tp_members*/
-  0,                           /*tp_getset*/
-  0,                           /*tp_base*/


Can you leave the zeroes? I think explicit is better than implicit in this case.

dreiss · 2018-10-09T21:15:29Z

_re2.cc

+
+static PyObject* regexp_pattern_get(RegexpObject2* self)
+{
+  return self->pattern;


Why doesn't this incref?

Hm, you are correct this should increase the reference count. Not sure why/how I missed it but I'll fix it!

fim · 2018-10-12T06:05:20Z

I tried various python versions (3.5, 3.6 and 3.7) and the segfault only happens in 3.7. I'll take a look into this and look at what might have changed in python3.7 to cause this behavior.

fim · 2018-10-12T22:18:44Z

I think PyObject_New() up to python3.6 initialized all the fields defined in the TYPE object that we pass to it which resulted in all PyObject * fields defined in the RegexpObject2 struct to be null by default. For some reason it looks like the value for groupindex in python3.7 is initialized to be 0x1 which in turn segfaults when we call Py_XDECREF() for it since we expect either NULL or a valid pointer.

We can easily work around that by nulling the field after PyObject_New() but I want to dig a bit deeper first in order to understand why that is happening and whether it might indicate there is a more systematic problem with either our approach or something in Python's object initialization process.

On the bright side, while testing on python-3.7, I noticed the following:

Python 3.7.0 (default, Oct 12 2018, 06:58:16) 
[...]
In [1]: import re                                                                                                                                                                                                   
In [2]: import re2                                                                                                                                                                                                  
In [3]: r2 = re2.compile('\w+')                                                                                                                                                                                     
In [4]: r = re.compile('\w+')                                                                                                                                                                                       

In [5]: %timeit r.match('foo bar')                                                                                                                                                                                  
319 ns ± 1.59 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [6]: %timeit r2.match('foo bar')                                                                                                                                                                                 
199 ns ± 2.4 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

fim · 2018-10-25T12:07:57Z

I haven't had the chance to look into the new way memory allocation/reference handling works in Python 3.7 yet (life and work got in the way) so I've updated the PR with the fix and the suggested changes (removed style fixes, readded the empty struct members for PyTypeObjects etc).

I'll try to look into the python object initialization at another time but I don't think that should hold up this set of changes.

@ivan can you check to see if you get any issues with this latest version?

fim · 2018-11-05T16:29:12Z

I also checked with python core, their suggestion was: if we need the field to be NULL, mark it as NULL explicitly which is what I did. Otherwise, we could use tp_alloc which initializes memory as well but that'll be slower.

@dreiss do you mind taking another look ?

dreiss · 2018-11-06T19:50:04Z

_re2.cc

+	  // consistent across Py3 and Py2.
+	  PyObject* index = PyLong_FromLong(it->second);
+	  if (index == NULL) {
+		break;


I think we want to immediately return NULL (after decref) here instead of storing an incomplete groupindex in self.

dreiss · 2018-11-06T19:50:37Z

_re2.cc

+		break;
+	  }
+    }
+    Py_INCREF(groupindex);


Why? Shouldn't we transfer the reference that we got from PyDict_New?

_re2.cc

As part of a quick test I did to measure the perfomance of pyre2, I noticed thathat we spent a lot of time allocating objects and that we could improve thethe performance simply by altering some of the C code to limit unnecessary object allocations. The main culprit was Py_BuildValue but it seems like the changes had a positive effect overall in terms of performance. As part of that I did: * Deleted code that populated attributes of the __dict__ struct * Deleted code that initialized the struct itself

fim · 2018-11-06T20:49:54Z

Addressed latest comments.

Changelog: * Performance improvements facebook#11 * Bugfixes facebook#12

Changelog: * Performance improvements #11 * Bugfixes #12

facebook-github-bot added the CLA Signed label Jul 30, 2018

fim force-pushed the re2_speedup branch from c5684a5 to e17225b Compare July 31, 2018 11:25

dreiss requested changes Aug 7, 2018

View reviewed changes

fim force-pushed the re2_speedup branch from e17225b to 3ad3512 Compare August 9, 2018 16:16

ivan added a commit to ArchiveTeam/grab-site that referenced this pull request Oct 9, 2018

Use facebook/pyre2 because ludios/pyre2 has a buggy PR applied:

e6b1709

facebook/pyre2#11 (comment)

dreiss requested changes Oct 9, 2018

View reviewed changes

fim force-pushed the re2_speedup branch from 3ad3512 to 88cfd95 Compare October 25, 2018 12:05

fim force-pushed the re2_speedup branch 2 times, most recently from 3555007 to 1f42a38 Compare November 5, 2018 08:15

fim force-pushed the re2_speedup branch from 1f42a38 to 160dcb6 Compare November 6, 2018 07:18

dreiss requested changes Nov 6, 2018

View reviewed changes

fim force-pushed the re2_speedup branch from 160dcb6 to 85eaa8c Compare November 6, 2018 20:49

dreiss merged commit 0d4980c into facebook:master Nov 6, 2018

noxiouz added a commit to noxiouz/pyre2 that referenced this pull request Jan 22, 2019

Update version to 1.0.7

c4e25ad

Changelog: * Performance improvements facebook#11 * Bugfixes facebook#12

noxiouz mentioned this pull request Jan 22, 2019

Update version to 1.0.7 #15

Merged

dreiss pushed a commit that referenced this pull request Feb 5, 2019

Update version to 1.0.7

edc437f

Changelog: * Performance improvements #11 * Bugfixes #12

		@@ -96,7 +119,6 @@ static PyObject* regexp_set_add(RegexpSetObject2* self, PyObject* pattern);
		static PyObject* regexp_set_compile(RegexpSetObject2* self);
		static PyObject* regexp_set_match(RegexpSetObject2* self, PyObject* text);

Performance improvements for pyre2 #11

Performance improvements for pyre2 #11

Conversation

fim commented Jul 30, 2018

dreiss commented Jul 30, 2018

fim commented Jul 30, 2018

dreiss commented Jul 30, 2018

fim commented Jul 31, 2018

fim commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fim commented Aug 9, 2018

fim commented Oct 1, 2018

ivan commented Oct 9, 2018

dreiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fim commented Oct 12, 2018

fim commented Oct 12, 2018

fim commented Oct 25, 2018

fim commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fim commented Nov 6, 2018