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.

Sign up for GitHub

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

Jump to bottom

perf(parser): use memchr for lexing comments #8193

Merged
merged 5 commits into from Oct 27, 2023
Merged

perf(parser): use memchr for lexing comments #8193

merged 5 commits into from Oct 27, 2023

Conversation

sno2
Copy link
Contributor

@sno2 sno2 commented Oct 25, 2023

Summary

This might improve the performance of lexing. Not sure, though.

Test Plan

This was tested using the existing tests.

@sno2
perf(parser): use memchr for lexing comments
410f157 
We now use memchr on its SIMD-supported targets to improve the
performance of lexing comments.
@github-actions
Copy link

github-actions bot commented Oct 25, 2023 •
edited

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@sno2 sno2 changed the title perf(parser): use memchr to improve parsing perf(parser): use memchr for lexing comments Oct 25, 2023
@sno2
Copy link
Contributor Author

sno2 commented Oct 25, 2023

Improvements seem neglible: https://codspeed.io/astral-sh/ruff/branches/sno2:main

@sno2 sno2 closed this Oct 25, 2023
MichaReiser
MichaReiser reviewed Oct 25, 2023
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment •
edited

Choose a reason for hiding this comment

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

It gives us a small speed up for the lexer. I didn't expect parsing to improve, because it performs way too many allocations. However, this could speed up things once we improve our parser.

crates/ruff_python_parser/src/lexer.rs Outdated
Comment on lines 410 to 414
#[cfg(any(
target_arch = "x86_64",
target_arch = "aarch64",
target_arch = "wasm32"
))]
Copy link
Member

Choose a reason for hiding this comment

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

I think memchr handl this automatically and not having the configuration ensures that newly supported platforms automatically profit from a faster memchr routine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, will look into this more.

@sno2 sno2 reopened this Oct 25, 2023
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 25, 2023 •
edited

CodSpeed Performance Report

Merging #8193 will not alter performance

Comparing sno2:main (0d3717f) with main (6f31e9c)

Summary

✅ 25 untouched benchmarks

@sno2
always use memchr
5981046
@sno2
perf(parser): try using compact_str for comments
0578954 
This might improve performance here because we do not compare comments
to values that often. Therefore, the memory locality might improve
performance in certain cases as well.
@sno2
Revert "perf(parser): try using compact_str for comments"
0d3717f 
Using compact_str does not seem to be faster for us :(
@sno2 sno2 marked this pull request as ready for review October 25, 2023 15:17
@sno2
Copy link
Contributor Author

sno2 commented Oct 25, 2023 •
edited

I've given up on trying to optimize the String creation so this is ready for review :^) It seems that the only way we could get a performance boost from comment string allocation in the lexer is to not do it at all and use lifetimes and &strs.

@MichaReiser
Copy link
Member

MichaReiser commented Oct 27, 2023 •
edited

I've given up on trying to optimize the String creation so this is ready for review :^) It seems that the only way we could get a performance boost from comment string allocation in the lexer is to not do it at all and use lifetimes and &strs.

I once had a pr that used SmolStr instead of String in the lexer to speed up things, but it didn't improve performance. I suspected that it was mainly because our string parsing is so slow (and allocates all over the place). It might be worth to give this another try after the string parsing rework lands

@MichaReiser MichaReiser added the parser Related to the parser label Oct 27, 2023
MichaReiser
MichaReiser approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you :)

@MichaReiser MichaReiser merged commit e2b5c6a into astral-sh:main Oct 27, 2023
17 checks passed
@miccal miccal mentioned this pull request Nov 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees
No one assigned
Labels
parser Related to the parser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sno2 @MichaReiser