Skip to content

bpo-28293: The regex cache no longer completely dump when full. #3768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Sep 26, 2017

@serhiy-storchaka serhiy-storchaka merged commit 114454e into python:master Sep 26, 2017
@serhiy-storchaka serhiy-storchaka deleted the re_cache_ordered_dict_popitem branch September 26, 2017 16:47
@@ -281,7 +288,10 @@ def _compile(pattern, flags):
p = sre_compile.compile(pattern, flags)
if not (flags & DEBUG):
if len(_cache) >= _MAXCACHE:
_cache.clear()
try:
_cache.popitem(False)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry that I'm late to the game in reviewing this, and thanks for fixing this. One comment, should this be

_cache.popitem(last=False)

for readability? (I think if history had been different, we would have made that a keyword-only argument.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. This wouldn't harm performance, because sre_compile.compile() few lines above is much more expensive. Open a new PR for changing this if you will.

Copy link
Member

Choose a reason for hiding this comment

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

I'm reworking my deferred compilation branch to add a new function, so I might just fold this simple change into that branch.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind; I'm abandoning the deferred compilation branch, so #3791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants