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

Skip costly attribute check for unicode strings #434

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

tonybaloney
Copy link
Contributor

This PR reduces the overall call time to escape() by about 40% when the input is a string by not calling PyObject_GetAttr() unless it isn't a unicode object.

PyObject_GetAttr is one of the most expensive calls in the C-API, compared with PyUnicode_CheckExact which is an interned pointer comparison (very fast).

Before:

screenshot 2024-04-19 at 14 10 56

After:

screenshot 2024-04-19 at 14 06 29

@tonybaloney
Copy link
Contributor Author

Related #433

@davidism
Copy link
Member

Thanks, I was just trying the same thing. Since escaping plain strings is probably the vast majority of cases, I added a if (PyUnicode_CheckExact(text) block to the very start which can return early. That seems like the best optimization, do exactly one check that will probably pass. I did notice that the __html__ block could be moved down as well.

("long plain", '"Hello, World!" * 1000'),
("long suffix", '"<strong>Hello, World!</strong>" + "x" * 100_000'),
):
for _ in range(1_000_000):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with micro benchmarks I find that calling the function 1000's of times in a loop helps reduce the noise. This extra file isn't needed but it's what I used to measure the difference

@davidism davidism added this to the 3.0.0 milestone Apr 19, 2024
co-authored-by: David Lord <davidism@gmail.com>
@davidism
Copy link
Member

I removed the extra bench script, added a change entry, and switched to an immediate PyUnicode_CheckExact as described in my last comment.

@davidism davidism merged commit 9941bf2 into pallets:main Apr 19, 2024
11 checks passed
@davidism
Copy link
Member

Here's a follow up where I actually move the checks, string conversion, and Markup wrapping into Python. No significant change in benchmark, simpler code. #437

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2024
@tonybaloney tonybaloney deleted the opt_native branch June 27, 2024 22:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants