Skip to content

gh-91524: Speed up the regular expression substitution - #91525

Merged
gpshead merged 11 commits into
python:mainfrom
serhiy-storchaka:re-compile-template
Oct 23, 2022
Merged

gh-91524: Speed up the regular expression substitution#91525
gpshead merged 11 commits into
python:mainfrom
serhiy-storchaka:re-compile-template

Conversation

@serhiy-storchaka

@serhiy-storchakaserhiy-storchaka commented Apr 14, 2022

Copy link
Copy Markdown
Member

Functions re.sub() and re.subn() and corresponding re.Pattern methods
are now 2-3 times faster for replacement strings containing group references.

Closes#91524

Functions re.sub() and re.subn() and corresponding re.Pattern methods
are now 2-3 times faster for replacement strings containing group references.
Comment threadModules/_sre/sre.c Outdated
Comment threadModules/_sre/sre.c
Comment threadModules/_sre/sre.c
Comment threadModules/_sre/sre.c
@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@ghost

Copy link
Copy Markdown

If use _PyUnicodeWriter/_PyBytesWriter, will it be faster?

There are two data contracts that crossing functions:

  1. between _parser.parse_template() and _sre.template(). List must be [literal, (group, literal)*]
  2. between _sre.template() and expand_template(). chunks = 1 + sum(1+bool(item.literal!=NULL) for item in items)

Such data contracts increase the difficulty of code maintenance. If use _PyUnicodeWriter/_PyBytesWriter, these data contracts are not necessary. And change TemplateObject like this:

typedefstruct {
PyObject_VAR_HEADstruct {
inttype; // literal or groupPyObject*item;
} items[0];
} TemplateObject;

In the current PR, adjacent groups also waste space.

Comment threadLib/re/__init__.py
Comment threadLib/re/_constants.py Outdated
Comment threadModules/_sre/sre.c Outdated
Comment threadModules/_sre/sre.c Outdated
Comment threadModules/_sre/sre.c Outdated
Comment threadLib/re/__init__.py
Comment threadLib/re/_constants.py Outdated
Comment threadLib/re/_parser.py
Comment threadModules/_sre/sre.c Outdated
Comment threadModules/_sre/sre.c
Comment threadModules/_sre/sre.c
Comment threadModules/_sre/sre.c Outdated
Comment threadModules/_sre/sre.c Outdated
Comment threadModules/_sre/sre.c
@serhiy-storchaka

Copy link
Copy Markdown
MemberAuthor

If use _PyUnicodeWriter/_PyBytesWriter, will it be faster?

I do not think so. The benefit of _PyUnicodeWriter/_PyBytesWriter is that we do not need to allocate an intermediate list and resize it multiple times if the number of items is not known. But in our case it is known, and in common case the buffer allocated on the stack is used. _PyUnicode_JoinArray should be faster because it knows ahead the length and the kind of the result string. _PyUnicodeWriter can perform several memory re-allocations.

I may try to experiment with _PyBytesWriter later. It will complicate the code, so I am not sure it is worth it.

@serhiy-storchaka

Copy link
Copy Markdown
MemberAuthor

Thank you for review. The conditions of my sight make my to easily miss some details.

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

Comment threadModules/_sre/sre.c
@serhiy-storchaka

Copy link
Copy Markdown
MemberAuthor

Until there is a clear answer, I plan to suspend work with Serhiy Storchaka.

Please do not do this. Your help compensates my disadvantage.

@ghost

ghost commented Apr 15, 2022

Copy link
Copy Markdown

It doesn't matter, the worst result is to postpone it to a later version.

There are a few possible changes about re module before 3.11 Beta1, #91495, #32411, #91477. Maybe you have other issues to deal with.
I'm afraid if you try too hard, it may be dangerous to your health.

@gpshead

Copy link
Copy Markdown
Member

@animalize - While it is great you are concerned for others health, it comes across poorly to say things in an "ultimatum" style such as a "threat" to stop working with someone because they've implied they may have a health concern. This is not kind. In many cultures (including Python's) that conduct is seen as discrimination. It limits others opportunities based on your own presumption of their abilities.

Among committers (which @serhiy-storchaka is a long time member of) none of us expect the steering council to micromanage our lives and most of us would rightfully reject that concept if we were to try. That isn't our purpose. While asking me or the SC to treat another committer specially was presumably voiced out of genuine concern here, it can come across as a put down that belittles their work even if this is not what you intended. We trust everyone to understand their own contribution abilities and act accordingly as they see fit.

Code review collaboration is a way for all of us to fill in gaps in that we all have in our work. Nobody is perfect.

(I'm replying with the above in this forum so that this response is visible to the same audience as the earlier messages that led to it.)

Lets ultimately stay focused on the PR here. Thanks for your reviews!

@ghost

ghost commented Apr 17, 2022

Copy link
Copy Markdown

Sorry. I don't know what to say.

I tried to improve this PR locally, IMHO the data contracts and the buffer[10] optimization make the code harder to understand. (Now I'm working on #91616)

@ghost

Copy link
Copy Markdown

I'll send a PR to serhiy-storchaka:re-compile-template later.

@eendebakpt

Copy link
Copy Markdown
Contributor

@serhiy-storchaka This PR has been approved by one other core dev, but not merged for some reason. The performance improvement seems worthwhile. Is there a reason why the branch has not been updated? If needed I can help to update the branch to current main

@gpshead
gpshead merged commit 75a6fad into python:mainOct 23, 2022
@gpshead

Copy link
Copy Markdown
Member

Thanks Serhiy & reviewers!

@serhiy-storchaka
serhiy-storchaka deleted the re-compile-template branch October 24, 2022 05:55
@serhiy-storchaka

Copy link
Copy Markdown
MemberAuthor

Thank you Gregory for the final polishing.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performancePerformance or resource usagetopic-regex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Speed up regular expression substitution

6 participants

@serhiy-storchaka@bedevere-bot@gpshead@eendebakpt@rhettinger@AlexWaygood