GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-91524: Speed up the regular expression substitution by serhiy-storchaka · Pull Request #91525 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Apr 14, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

  • Issue: Speed up regular expression substitution #91524

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thinkwelltwd, kumaraditya303, hauntsaninja, erlend-aasland, m-aciek, eandersson, saito828koki, haniffm, and machineaaaax reacted with hooray emoji
All reactions
  • 🎉 9 reactions
@serhiy-storchaka
pythongh-91524: Speed up the regular expression substitution
c213b37
Functions re.sub() and re.subn() and corresponding re.Pattern methods
are now 2-3 times faster for replacement strings containing group references.
@bedevere-bot bedevere-bot added the awaiting core review label Apr 14, 2022
@AlexWaygood AlexWaygood added performance Performance or resource usage topic-regex labels Apr 14, 2022
@blurb-it
📜🤖 Added by blurb_it.
50c435f
gpshead
gpshead requested changes Apr 14, 2022
View reviewed changes
Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting core review label Apr 14, 2022
@bedevere-bot

bedevere-bot commented Apr 14, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting changes label Apr 14, 2022
@ghost

ghost commented Apr 15, 2022

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:

typedef struct {
    PyObject_VAR_HEAD
    struct {
        int type; // literal or group
        PyObject *item;
    } items[0];
} TemplateObject;

In the current PR, adjacent groups also waste space.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ghost
ghost reviewed Apr 15, 2022
View reviewed changes
Comment thread Lib/re/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/re/_constants.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added 2 commits April 15, 2022 09:32
@serhiy-storchaka
Address review comments.
a47446d
@serhiy-storchaka
Merge branch 're-compile-template' of github.com:serhiy-storchaka/cpy…
5a7f874
…thon into re-compile-template
serhiy-storchaka
serhiy-storchaka commented Apr 15, 2022
View reviewed changes
Comment thread Lib/re/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/re/_constants.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/re/_parser.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Apr 15, 2022

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Apr 15, 2022

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Apr 15, 2022
@bedevere-bot

bedevere-bot commented Apr 15, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested a review from gpshead April 15, 2022 06:47
ghost
ghost reviewed Apr 15, 2022
View reviewed changes
Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Apr 15, 2022

Copy link
Copy Markdown
Member Author

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

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

gpshead, ambv, and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Apr 15, 2022 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
Merge branch 'main' into re-compile-template
7bb228f
@gpshead

gpshead commented Apr 16, 2022

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!

eendebakpt, AlexWaygood, erlend-aasland, and 5j9 reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Apr 17, 2022 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Apr 19, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka mentioned this pull request Apr 19, 2022
Reorganize the re module sources #91308
Closed
rhettinger
rhettinger approved these changes May 10, 2022
View reviewed changes
@bedevere-bot bedevere-bot removed the awaiting change review label May 10, 2022
@bedevere-bot bedevere-bot added the awaiting merge label May 10, 2022
@eendebakpt

eendebakpt commented Oct 23, 2022

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead added 5 commits October 23, 2022 20:40
@gpshead
Merge branch 'main' into re-compile-template
2271aa8
@gpshead
remove ^M dos line endings
78dc027
@gpshead
correct mis-merge in import()
7ab7ade
@gpshead
remove import(), an internal API exists for that.
7eebbff
@gpshead
improve the _sre.template docstring.
00d5e12
gpshead
gpshead approved these changes Oct 23, 2022
View reviewed changes
@gpshead
:gh: instead of :issue:
1c81744
@gpshead
gpshead merged commit 75a6fad into python:main Oct 23, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 23, 2022
@gpshead

gpshead commented Oct 23, 2022

Copy link
Copy Markdown
Member

Thanks Serhiy & reviewers!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

serhiy-storchaka commented Oct 24, 2022

Copy link
Copy Markdown
Member Author

Thank you Gregory for the final polishing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@gpshead gpshead gpshead approved these changes
@rhettinger rhettinger rhettinger approved these changes
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance Performance or resource usage topic-regex

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Speed up regular expression substitution

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@serhiy-storchaka @bedevere-bot @gpshead @eendebakpt @rhettinger @AlexWaygood
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.