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 — [WIP] bpo-23689: re module, fix memory leak when a match is terminated by a signal · Pull Request #32188 · python/cpython

Conversation

@ghost

@ghost ghost commented Mar 30, 2022 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

A draft

Count the number of REPEAT when compiling a pattern, and allocate an array in SRE_STATE.
At any time, a REPEAT will have at most one in active, so a SRE_REPEAT array is fine.

https://bugs.python.org/issue23689

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added the awaiting review label Mar 30, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 30, 2022
@wjssz
re: fix memory leak when a match is terminated by a signal
652d87c
Count the number of REPEAT when compiling a pattern, and allocate an array in `SRE_STATE`.
At any time, a REPEAT will have at most one in active, so a `SRE_REPEAT` array is fine.
@ghost

ghost commented Mar 30, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

@serhiy-storchaka
This is a draft. If use 5~7 days, it may achieve better quality.
I can wait until your PR "bpo-47152: Convert the re module into a package" is merged, then I redo this PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka
serhiy-storchaka reviewed Mar 30, 2022
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Seems repeat_index is only needed for REPEAT.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/sre_lib.h Resolved

Uh oh!

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

Comment thread Modules/sre_lib.h Resolved

Uh oh!

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

wjssz added 3 commits March 30, 2022 18:14
@wjssz
don't emit repeat_index for REPEAT_ONE, MIN_REPEAT_ONE, POSSESSIVE_RE…
d2b505c
…PEAT_ONE
@wjssz
print repeat_index in TRACE()
adedb24
@wjssz
use PyMem_New instead of PyMem_RawCalloc
e8567e1
Same as the code above here, maybe a bit more cache friendly.
@ghost

ghost commented Mar 30, 2022

Copy link
Copy Markdown
Author

This PR has not been carefully checked yet, it will take a few days to do this.

Just want to ask your arrangement, is this PR merged first, or your PR first?
I tend to merge your PR first, then I can carefully check when you are preparing your PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented Mar 30, 2022

Copy link
Copy Markdown
Member

I prefer to wait until this PR is merged. It is easy to redo my changes.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Mar 30, 2022

Copy link
Copy Markdown
Author

Ok, I will finish this PR in a few days.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka
serhiy-storchaka reviewed Mar 30, 2022
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What I propose:

  1. Remove repeat_index in POSSESSIVE_REPEAT.
  2. Remove REPEAT count in INFO and pass it as a separate argument to _sre.compile().
  3. Validate repeat_index for REPEAT in _validate_inner().

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/sre_lib.h

case SRE_OP_POSSESSIVE_REPEAT:
/* create possessive repeat contexts. */
/* <POSSESSIVE_REPEAT> <skip> <1=min> <2=max> pattern

@serhiy-storchaka serhiy-storchaka Mar 30, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What about POSSESSIVE_REPEAT? repeat_index is not used here. MAX_UNTIL and MIN_UNTIL are not used with POSSESSIVE_REPEAT, so there is no problem with keeping POSSESSIVE_REPEAT intact.

Sorry, something went wrong.

Uh oh!

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

All reactions

@ghost ghost Mar 30, 2022

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If I add more conditional checks, I'm afraid to slow down the pattern compilation.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Mar 30, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not much. Checking if op is POSSESSIVE_REPEAT is cheap in comparison with parsing or optimization.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/sre_compile.py
Comment on lines +594 to +596
# REPEAT count
assert len(code) == _REPEAT_COUNT_OFFSET
emit(0)

@serhiy-storchaka serhiy-storchaka Mar 30, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It seems to me that INFO is only used for optimization. If you ignore it, all will work, but maybe slower.

Essential information like the number of groups etc is passed to _sre.compile() as separate arguments. The REPEAT count is of such kind. If it is not known we cannot start matching.

I propose to pass it as a separate argument. And in _validate_inner() check that all repeat_index are less than that count (like we validate group indices and the MARK argument).

Sorry, something went wrong.

Uh oh!

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

All reactions

@ghost ghost Mar 30, 2022

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sounds reasonable, I'll do these.
Please merge your PR first.

Sorry, something went wrong.

Uh oh!

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

All reactions
@ghost

ghost commented Mar 30, 2022

Copy link
Copy Markdown
Author

In _compile() recursive function, it seems hard to return the number of REPEATs.

   def _compile(code, pattern, flags):
       # `code` is a list object
       # `pattern` and `flags` are input data
       ...

For counting the number of REPEATs, it's better at compile-time than parse-time.
Is it worth adding a parameter to _compile() function for this purpose?

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented Mar 30, 2022

Copy link
Copy Markdown
Member

You can save it as an attribute of pattern.state.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Mar 30, 2022

Copy link
Copy Markdown
Author

You can save it as an attribute of pattern.state.

In _compile() function, pattern.state is not accessable. pattern is SubPattern.data, not SubPattern.
I have an idea, I will try tommorw, time to sleep.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Mar 31, 2022 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

CompileData.diff changes code to a CompileData object, which may save intermediate states during compilation. Only apply this patch to main branch, MSVC2022 non-PGO release build runs test_re.py is 2.1% slower (0.277 sec -> 0.283 sec).

Another way is to temporarily store it in a field in SRE_OP_INFO, but this would make SRE_OP_INFO take up an unnecessary field.

To be honest, the whole patch is intricacy, so reviewing is harder than making it from zero. If you think so too, you may make a PR instead of this PR.

CompileData.diff.txt (.diff attachment is not supportted by GitHub, so I added .txt)

edit: below patch is faster a bit.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Mar 31, 2022

Copy link
Copy Markdown
Author

This simpler patch is faster (2.80 sec)
simple_patch.diff.txt

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Mar 31, 2022

Copy link
Copy Markdown
Author

I can divide the patch into some commits, each commit only modify one simple step, which makes the review much easier. Then this PR will be closed.
I suggest merge your PR first.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost ghost closed this Apr 1, 2022
@ghost
ghost deleted the repeat_count branch April 4, 2022 06:11
@serhiy-storchaka serhiy-storchaka mentioned this pull request Apr 13, 2022
Reorganize the re module sources #91308
Closed
@abacabadabacaba abacabadabacaba mannequin mentioned this pull request Jun 15, 2022
Memory leak in Modules/sre_lib.h #67877
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

@serhiy-storchaka @the-knights-who-say-ni @bedevere-bot @wjssz
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.