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

perf(lexer): optimize Source #8295

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Jan 6, 2025

SourcePosition has a couple of invariants:

  • Any SourcePosition must be in bounds of Source.
  • Distance between any 2 SourcePositions cannot exceed u32::MAX, because that's the upper limit on size of source text.

However, prior to this PR, all compiler could see is pointers which could be anything. It wasn't able to understand the invariants, or utilize them for efficient codegen.

This PR makes compiler aware of these invariants in 2 ways:

  1. Replace comparing pointers with >, < etc, and calculating offsets via ptr as usize calculations with std::ptr::offset_from. offset_from has safety invariants which match Source's.
  2. Calculate offsets between SourcePositions with u32::try_from(distance).unwrap_unchecked(). This informs compiler offset is definitely non-negative and <= u32::MAX.

Copy link
Contributor Author

overlookmotel commented Jan 6, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added A-parser Area - Parser C-performance Category - Solution not expected to change functional behavior, only performance labels Jan 6, 2025
Copy link

codspeed-hq bot commented Jan 6, 2025

CodSpeed Performance Report

Merging #8295 will not alter performance

Comparing 01-01-perf_lexer_optimize_source_ (703d983) with main (e1f8ea4)

Summary

✅ 29 untouched benchmarks

@overlookmotel overlookmotel marked this pull request as ready for review January 6, 2025 13:39
@overlookmotel overlookmotel force-pushed the 01-01-perf_lexer_optimize_source_ branch from 5a91661 to 5270ad1 Compare January 6, 2025 13:41
@overlookmotel overlookmotel marked this pull request as draft January 6, 2025 13:41
@overlookmotel
Copy link
Contributor Author

Ha! It actually seems to reduce perf slightly, according to CodSpeed. Probably it's the usize to u32 conversion in offsets. I'll try to remove that overhead.

@overlookmotel overlookmotel force-pushed the 01-06-fix_lexer_source_is_not_clone_ branch from 8442546 to 727ab4e Compare January 6, 2025 17:57
@overlookmotel overlookmotel force-pushed the 01-01-perf_lexer_optimize_source_ branch from 5270ad1 to 5eeb81f Compare January 6, 2025 17:57
@overlookmotel overlookmotel force-pushed the 01-06-fix_lexer_source_is_not_clone_ branch from 727ab4e to c07b69c Compare January 6, 2025 19:52
@overlookmotel overlookmotel force-pushed the 01-01-perf_lexer_optimize_source_ branch from 5eeb81f to 645092c Compare January 6, 2025 19:52
@overlookmotel overlookmotel force-pushed the 01-06-fix_lexer_source_is_not_clone_ branch from c07b69c to de208a9 Compare January 6, 2025 20:09
@overlookmotel overlookmotel force-pushed the 01-01-perf_lexer_optimize_source_ branch from 645092c to 134a26c Compare January 6, 2025 20:10
@Boshen Boshen changed the base branch from 01-06-fix_lexer_source_is_not_clone_ to graphite-base/8295 January 7, 2025 07:00
@Boshen Boshen force-pushed the graphite-base/8295 branch from de208a9 to e1f8ea4 Compare January 7, 2025 07:15
@Boshen Boshen force-pushed the 01-01-perf_lexer_optimize_source_ branch from 134a26c to c691429 Compare January 7, 2025 07:15
@Boshen Boshen changed the base branch from graphite-base/8295 to main January 7, 2025 07:16
@Boshen Boshen force-pushed the 01-01-perf_lexer_optimize_source_ branch from c691429 to 703d983 Compare January 7, 2025 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area - Parser C-performance Category - Solution not expected to change functional behavior, only performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant