GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||
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
|
|
@serhiy-storchaka |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Same as the code above here, maybe a bit more cache friendly.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I prefer to wait until this PR is merged. It is easy to redo my changes. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ok, I will finish this PR in a few days. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
What I propose:
_sre.compile()._validate_inner().
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| case SRE_OP_POSSESSIVE_REPEAT: | ||
| /* create possessive repeat contexts. */ | ||
| /* <POSSESSIVE_REPEAT> <skip> <1=min> <2=max> pattern |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
| # REPEAT count | ||
| assert len(code) == _REPEAT_COUNT_OFFSET | ||
| emit(0) |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
In 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
You can save it as an attribute of |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
In |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Another way is to temporarily store it in a field in 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.
edit: below patch is faster a bit. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This simpler patch is faster (2.80 sec) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.