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-96346: Use double caching for re._compile() by serhiy-storchaka · Pull Request #96347 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Aug 27, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Use double caching for re._compile() #96346

Sorry, something went wrong.

Uh oh!

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

All reactions
@serhiy-storchaka serhiy-storchaka added the performance Performance or resource usage label Aug 27, 2022
@serhiy-storchaka
serhiy-storchaka requested a review from rhettinger August 27, 2022 20:18
@bedevere-bot bedevere-bot added the awaiting core review label Aug 27, 2022
@serhiy-storchaka
pythongh-96346: Use double caching for re._compile()
9151549
@serhiy-storchaka
serhiy-storchaka force-pushed the re-compile-cache branch from 5c69476 to 9151549 Compare August 27, 2022 20:20
@rhettinger

rhettinger commented Aug 27, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This looks like a reasonable change.

Consider whether OrderedDict should be used in the cache where the order changes. We've shown that next(iter(d)) is quadratic while od.popitem(False) is linear. For the cache sizes in the re module, this might not matter or might be outweighed by OrderedDict's slower __getitem__ and __setitem__.

It is fine to go ahead with this PR since it is improves on what we have now.

All reactions

Sorry, something went wrong.

Uh oh!

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

rhettinger
rhettinger approved these changes Aug 27, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 27, 2022
@serhiy-storchaka

serhiy-storchaka commented Aug 28, 2022

Copy link
Copy Markdown
Member Author

It was discussed in #76519, performance difference is negligible. On other hand, OrderedDict adds a dependence. And it would be much worse if it is implemented in Python.

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka
Add some comments.
5fe3232
@serhiy-storchaka

serhiy-storchaka commented Aug 28, 2022

Copy link
Copy Markdown
Member Author

I added some data in the issue.

All reactions

Sorry, something went wrong.

Uh oh!

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

rhettinger
rhettinger reviewed Aug 30, 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/__init__.py Outdated
# _cache2 uses the simple FIFO policy which has better latency.
# _cache uses the LRU policy which has better hit rate.
# OrderedDict is not used because it adds a new dependence, and
# performance difference is negligible.

@rhettinger rhettinger Aug 30, 2022

Copy link
Copy Markdown
Contributor

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.

I would omit the OrderedDict part of the comment. It is debatable and doesn't need to be in the code. The important part is the two lines before that explain the two caches.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Aug 31, 2022

Copy link
Copy Markdown
Member 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.

There should be an explanation why OrderedDict is not used at first place, no?

Also, I was afraid that some new contributor passing through the code and not aware about the history of this code can submit a PR with an "obvious" improvement, and it can be merged while I am not here. The history of this code contains many changes and reversions.

I removed this, and hope the new comment about next(iter(_cache)) will be enough.

Sorry, something went wrong.

Uh oh!

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

All reactions
@rhettinger

rhettinger commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

Side discussion: I was looking at the code in tomllib and its cache has maxsize=None. I wondered whether its use pattern is substantially different or whether the policy should be the same as for re. Perhaps the tomllib module should be bounded, or perhaps the re module didn't really need a limit. Another alternative is to set a limit that is very high so that most users never hit the limit and never trigger an eviction. After all, the objects being cached are very small, so setting a size limit is arguably a premature optimization. The code would be much simpler and a little faster without a size limit.

All reactions

Sorry, something went wrong.

Uh oh!

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

rhettinger
rhettinger reviewed Aug 30, 2022
View reviewed changes
Comment thread Lib/re/__init__.py Resolved

Uh oh!

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

@hukkin

hukkin commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

Side discussion: I was looking at the code in tomllib and its cache has maxsize=None. I wondered whether its use pattern is substantially different or whether the policy should be the same as for re

Hi there, it's tomllib author here! Tomllib's cache does not have an explicit bound, but is implicitly bound to 2880 items. This stems from the fact the the function's input always has to go through a regex before the lru_cached function is called.

The number 2880 comes from 24 hours * 60 minutes * 2 (offset direction).

rhettinger and serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@rhettinger

rhettinger commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

@hukkin I suggest adding a comment to that effect.

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka
Address review comments.
d619f6d
@serhiy-storchaka

serhiy-storchaka commented Aug 31, 2022

Copy link
Copy Markdown
Member Author

Thank you Raymond for looking at this yet one time.

As for the limit, I think it is needed here, because RE patterns can be created on-fly, depending on the user data, and some programs may use millions of different patterns, but every pattern is only used once.

All reactions

Sorry, something went wrong.

Uh oh!

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

ezio-melotti
ezio-melotti reviewed Sep 18, 2022
View reviewed changes
Comment thread Lib/re/__init__.py
p = _compiler.compile(pattern, flags)
if not (flags & DEBUG):

key = (type(pattern), pattern, flags)

@ezio-melotti ezio-melotti Sep 18, 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.

Can't this be defined before the try/except above?

I also wonder if an if key in _cache2: return _cache2[key] would be more efficient than the try/except.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Oct 5, 2022

Copy link
Copy Markdown
Member 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.

All this would add a small but measurable overhead in the common case. I tested this when I wrote the current implementation.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/re/__init__.py
if not (flags & DEBUG):

key = (type(pattern), pattern, flags)
p = _cache.pop(key, None)

@ezio-melotti ezio-melotti Sep 18, 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.

Why does it remove it from the _cache?
I think it would be better to add a comment to elaborate a bit.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Oct 5, 2022

Copy link
Copy Markdown
Member 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.

Because we need to move it to the end.

Sorry, something went wrong.

Uh oh!

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

ezio-melotti reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
serhiy-storchaka added 2 commits October 5, 2022 13:25
@serhiy-storchaka
Merge branch 'main' into re-compile-cache
077e6dd
@serhiy-storchaka
Add a comment.
bd618bf
@ambv
ambv merged commit c11b667 into python:main Oct 7, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 7, 2022
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
27a4980
* main: (38 commits)
  pythongh-92886: make test_ast pass with -O (assertions off) (pythonGH-98058)
  pythongh-92886: make test_coroutines pass with -O (assertions off) (pythonGH-98060)
  pythongh-57179: Add note on symlinks for os.walk (python#94799)
  pythongh-94808: Fix regex on exotic platforms (python#98036)
  pythongh-90085: Remove vestigial -t and -c timeit options (python#94941)
  pythonGH-83901: Improve Signature.bind error message for missing keyword-only params (python#95347)
  pythongh-61105: Add default param, note on using cookiejar subclass (python#95427)
  pythongh-96288: Add a sentence to `os.mkdir`'s docstring. (python#96271)
  pythongh-96073: fix backticks in NEWS entry (pythonGH-98056)
  pythongh-92886: [clinic.py] raise exception on invalid input instead of assertion (pythonGH-98051)
  pythongh-97997: Add col_offset field to tokenizer and use that for AST nodes (python#98000)
  pythonGH-88968: Reject socket that is already used as a transport (python#98010)
  pythongh-96346: Use double caching for re._compile() (python#96347)
  pythongh-91708: Revert params note in urllib.parse.urlparse table (python#96699)
  pythongh-96265: Fix some formatting in faq/design.rst (python#96924)
  pythongh-73196: Add namespace/scope clarification for inheritance section (python#92840)
  pythongh-97646: Change `.js` and `.mjs` files mimetype to conform to RFC 9239 (python#97934)
  pythongh-97923: Always run Ubuntu SSL tests with others in CI (python#97940)
  pythongh-97956: Mention `generate_global_objects.py` in `AC How-To` (python#97957)
  pythongh-96959: Update HTTP links which are redirected to HTTPS (python#98039)
  ...
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@serhiy-storchaka @mpage
pythongh-96346: Use double caching for re._compile() (python#96347)
053555c
@hukkin hukkin mentioned this pull request Oct 28, 2024
tomllib: Add a comment about implicit lru_cache bound #126078
Merged
@serhiy-storchaka
serhiy-storchaka deleted the re-compile-cache branch July 1, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger rhettinger approved these changes
@ezio-melotti ezio-melotti ezio-melotti left review comments

Uh oh!

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

Assignees

No one assigned

Labels

performance Performance or resource usage

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.

6 participants

@serhiy-storchaka @rhettinger @hukkin @ezio-melotti @ambv @bedevere-bot
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.