Skip to content

Cheap cloneable LineIndex #3896

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
Merged
MichaReiser merged 1 commit into from
Apr 11, 2023
Merged

Cheap cloneable LineIndex #3896

MichaReiser merged 1 commit into from
Apr 11, 2023

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Apr 6, 2023 •
edited
Loading

This PR extracts the Index type from Locator, changes it to use an Arc internally to make it cheaply cloneable, and makes it public.

The motivation for this change is that I want to output an advice with the recommended fix in the text and grouped emitter if --show-sources is true. Doing so requires access to the source code and the LineIndex to apply the edits.

I used this chance to change the Locator and LineIndex to return TextSize rather than usizes for byte offsets.

@MichaReiser MichaReiser mentioned this pull request Apr 6, 2023
Merged
@MichaReiser
Copy link
Member Author

MichaReiser commented Apr 6, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser force-pushed the extract-message-emitters branch from d9a3769 to ab2d3c2 Compare April 6, 2023 09:33
@MichaReiser MichaReiser force-pushed the extract-message-emitters branch from ab2d3c2 to 6d2ccf7 Compare April 6, 2023 09:42
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023 •
edited
Loading

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     15.1±0.12ms     2.7 MB/sec    1.00     15.1±0.09ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.8±0.01ms     4.4 MB/sec    1.00      3.8±0.00ms     4.4 MB/sec
linter/all-rules/numpy/globals.py          1.00    416.8±1.91µs     7.1 MB/sec    1.00    415.2±1.26µs     7.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.5±0.02ms     3.9 MB/sec    1.00      6.5±0.09ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.01      7.9±0.01ms     5.2 MB/sec    1.00      7.8±0.04ms     5.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1744.5±3.49µs     9.5 MB/sec    1.00   1741.8±1.84µs     9.6 MB/sec
linter/default-rules/numpy/globals.py      1.00    180.2±0.37µs    16.4 MB/sec    1.01    181.9±1.64µs    16.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.01ms     7.0 MB/sec    1.00      3.6±0.00ms     7.1 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     16.4±0.20ms     2.5 MB/sec    1.00     16.5±0.22ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.13ms     4.0 MB/sec    1.00      4.2±0.08ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00   499.8±14.60µs     5.9 MB/sec    1.00    497.9±8.88µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.9±0.10ms     3.7 MB/sec    1.00      6.9±0.09ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.3±0.11ms     4.9 MB/sec    1.00      8.2±0.07ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1812.6±24.35µs     9.2 MB/sec    1.00  1804.9±22.26µs     9.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    192.0±3.54µs    15.4 MB/sec    1.02    195.9±6.51µs    15.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.8±0.06ms     6.7 MB/sec    1.00      3.8±0.06ms     6.7 MB/sec

@MichaReiser MichaReiser force-pushed the extract-line-index branch 2 times, most recently from 8ce729d to b0f2c79 Compare April 6, 2023 10:09
@MichaReiser MichaReiser marked this pull request as ready for review April 6, 2023 10:10
This was referenced Apr 6, 2023
Merged
Merged
charliermarsh
charliermarsh approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Looks great.

charliermarsh
charliermarsh reviewed Apr 6, 2023
View reviewed changes
crates/ruff_python_ast/src/source_code/line_index.rs
/// Internally this is represented as a [`NonZeroUsize`], this enables some
/// memory optimizations
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct OneIndexed(NonZeroUsize);
Copy link
Member

@charliermarsh charliermarsh Apr 6, 2023

Choose a reason for hiding this comment

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

I've never used NonZeroUSize so I'll just ask: why do we need OneIndexed at all? Can we not use that type directly?

Copy link
Member Author

@MichaReiser MichaReiser Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could. But I prefer semantic wrapper types because they communicate intent and allow us to limit the API, add helper methods that are only meaningful in this context (e.g. to_zero_indexed) or use method names that are more adequate in this context.

The use of wrapper types further allows us to replace the internal representation if we want (e.g. use a NonZeroU32) or to search for all usages of that concept (we may want to use NonZeroUsize in other places that are entirely unrelated).

This was referenced Apr 7, 2023
Merged
Merged
Merged
@MichaReiser MichaReiser force-pushed the extract-message-emitters branch from f5193e6 to 2ba3c0d Compare April 11, 2023 07:16
Base automatically changed from extract-message-emitters to main April 11, 2023 07:24
@MichaReiser MichaReiser enabled auto-merge (squash) April 11, 2023 07:27
@MichaReiser MichaReiser merged commit 76c47a9 into main Apr 11, 2023
@MichaReiser MichaReiser deleted the extract-line-index branch April 11, 2023 07:33
@MichaReiser MichaReiser added the internal An internal refactor or improvement label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

No one assigned

Labels

internal An internal refactor or improvement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MichaReiser @charliermarsh