Skip to content

Comments

perf(lexer): optimize Source#8295

Closed
overlookmotel wants to merge 1 commit intomainfrom
01-01-perf_lexer_optimize_source_
Closed

perf(lexer): optimize Source#8295
overlookmotel wants to merge 1 commit intomainfrom
01-01-perf_lexer_optimize_source_

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Jan 6, 2025 •
edited
Loading

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.

This was referenced Jan 6, 2025
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@overlookmotel Graphite App
Copy link
Member Author

overlookmotel commented Jan 6, 2025 •
edited
Loading

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
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 6, 2025 •
edited
Loading

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
Member Author

overlookmotel commented Jan 6, 2025

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
@overlookmotel
Copy link
Member Author

overlookmotel commented Jan 15, 2025

This is not worth pursuing right now. At present, it's hurting perf slightly. If could fix it to make it improve perf, then likely that perf improvement would be small anyway. So it's not worthwhile putting any time into now.

Archived on overlookmotel/optimize-lexer-source-with-offset branch in case want to return to this later.

@overlookmotel overlookmotel deleted the 01-01-perf_lexer_optimize_source_ branch January 15, 2025 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

A-parser Area - Parser C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@overlookmotel