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

Avoid cloning Name when looking up function and class types #14092

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Nov 4, 2024

Summary

Uses Salsa's new Lookup functionality for FunctionType and ClassType names.
The Lookup trait allows looking-up interned values by a borrowed value. In this case
we can lookup the interned type using the borrowed str and only allocate the owned `Name`` when
the type doesn't already exist.

This was part of #14087 but I extracted it into its own PR to see if it is the reason for the significant incremental perf improvement.

Test Plan

cargo test

@MichaReiser MichaReiser added internal An internal refactor or improvement red-knot Multi-file analysis & type inference labels Nov 4, 2024
@MichaReiser MichaReiser merged commit bc0586d into main Nov 4, 2024
20 checks passed
@MichaReiser MichaReiser deleted the micha/lookup-name branch November 4, 2024 14:53
Copy link
Contributor

github-actions bot commented Nov 4, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@carljm
Copy link
Contributor

carljm commented Nov 4, 2024

This was part of #14087 but I extracted it into its own PR to see if it is the reason for the significant incremental perf improvement.

Was it? I don't see any CodSpeed results reported here.

@MichaReiser
Copy link
Member Author

Nope, this wasn't it :)

carljm added a commit to Glyphack/ruff that referenced this pull request Nov 5, 2024
* main: (39 commits)
  Also remove trailing comma while fixing C409 and C419 (astral-sh#14097)
  Re-enable clippy `useless-format` (astral-sh#14095)
  Derive message formats macro support to string (astral-sh#14093)
  Avoid cloning `Name` when looking up function and class types (astral-sh#14092)
  Replace `format!` without parameters with `.to_string()` (astral-sh#14090)
  [red-knot] Do not panic when encountering string annotations (astral-sh#14091)
  [red-knot] Add MRO resolution for classes (astral-sh#14027)
  [red-knot] Remove `Type::None` (astral-sh#14024)
  Cached inference of all definitions in an unpacking (astral-sh#13979)
  Update dependency uuid to v11 (astral-sh#14084)
  Update Rust crate notify to v7 (astral-sh#14083)
  Update cloudflare/wrangler-action action to v3.11.0 (astral-sh#14080)
  Update dependency mdformat-mkdocs to v3.1.1 (astral-sh#14081)
  Update pre-commit dependencies (astral-sh#14082)
  Update dependency ruff to v0.7.2 (astral-sh#14077)
  Update NPM Development dependencies (astral-sh#14078)
  Update Rust crate thiserror to v1.0.67 (astral-sh#14076)
  Update Rust crate syn to v2.0.87 (astral-sh#14075)
  Update Rust crate serde to v1.0.214 (astral-sh#14074)
  Update Rust crate pep440_rs to v0.7.2 (astral-sh#14073)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants