Skip to content
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

Use pre-compiled regular expressions #77

Merged
merged 4 commits into from
Oct 11, 2023

Conversation

rtobar
Copy link
Contributor

@rtobar rtobar commented Oct 10, 2023

Some low-hanging fruit inspired by #76.

These speed up linting by potentially big factors (e.g., linting
CPython/Docs/library/*.rst is ~25% faster locally).

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar rtobar mentioned this pull request Oct 10, 2023
Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM! This gets us most of the way to the speedup I reported in #76 (comment) :D

There's a few other things we can do to speed things up further, but I can tackle them in a followup PR tomorrow, since I've already got them around locally :)

@AlexWaygood
Copy link
Collaborator

A lot of precompiled regexes in this repo seem to be located in sphinxlint.rst; not sure if these should be as well. @hugovk, any thoughts on that?

@hugovk
Copy link
Collaborator

hugovk commented Oct 11, 2023

A lot of precompiled regexes in this repo seem to be located in sphinxlint.rst; not sure if these should be as well. @hugovk, any thoughts on that?

I think they're fine where they are, but no strong opinion either way :)

@AlexWaygood
Copy link
Collaborator

A lot of precompiled regexes in this repo seem to be located in sphinxlint.rst; not sure if these should be as well. @hugovk, any thoughts on that?

I think they're fine where they are, but no strong opinion either way :)

Let's leave them where they are for now!

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

thanks!

The latter seems to be what the current codebase calls Pattern objects,
so let's not add confusion.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Copy link
Contributor Author

rtobar commented Oct 11, 2023

Sorry, just added another one after another quick profiling, and renamed the _PATTERN objects to _RE, which is what the current codebase calls re.Pattern objects. I won't be adding anymore to this PR.

Copy link
Collaborator

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Running with main on CPython docs:

time sphinx-lint Doc
No problems found.
sphinx-lint Doc  17.89s user 0.31s system 672% cpu 2.705 total

And this PR:

time sphinx-lint Doc
No problems found.
sphinx-lint Doc  13.98s user 0.30s system 641% cpu 2.225 total

A nice 18% improvement! Again almost too easy!

@hugovk hugovk merged commit 1e84162 into sphinx-contrib:main Oct 11, 2023
@rtobar rtobar deleted the re-compilation branch October 11, 2023 14:16
@hugovk
Copy link
Collaborator

hugovk commented Oct 11, 2023

With both this and #79:

time sphinx-lint Doc
No problems found.
sphinx-lint Doc  11.93s user 0.31s system 626% cpu 1.954 total

27% improvement! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants