Skip to content

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

Merged
MichaReiser merged 5 commits intoastral-sh:mainfrom
sno2:main
Oct 27, 2023
Merged

perf(parser): use memchr for lexing comments#8193
MichaReiser merged 5 commits intoastral-sh:mainfrom
sno2:main

Conversation

@sno2
Copy link
Copy Markdown
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
Copy Markdown
Contributor

github-actions Bot commented Oct 25, 2023 •
edited
Loading

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

@MichaReiser MichaReiser left a comment •
edited
Loading

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.

Comment thread 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
Copy Markdown
Member

@MichaReiser MichaReiser Oct 25, 2023

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
Copy Markdown
Contributor Author

@sno2 sno2 Oct 25, 2023

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
Copy Markdown

codspeed-hq Bot commented Oct 25, 2023 •
edited
Loading

CodSpeed Performance Report

Merging #8193 will not alter performance

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

Summary

✅ 25 untouched benchmarks

sno2 added 4 commits October 24, 2023 23:33
@sno2
Merge branch 'main' of https://github.com/sno2/ruff
a0f2442
@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
Copy Markdown
Contributor Author

sno2 commented Oct 25, 2023 •
edited
Loading

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

MichaReiser commented Oct 27, 2023 •
edited
Loading

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
Copy Markdown
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
@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.

2 participants

@sno2 @MichaReiser