-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Add environment variable to dump Salsa memory usage stats #18928
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
Conversation
3582d8c to
bc95677
Compare
|
|
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great
I've a few smaller nits. The only downside of the design is that it's very easy to forget the heap_size attribute on a salsa query which will result in under counting. That makes me wonder if we should change the design in salsa so that the stack and heap_size is reported separately for each query (we can show a total as well) and the heap_size would be Unknown if the heap_size attribute isn't set. This would make it more appearant where heap_size attributes are missing (compared to, ah, this query doesn't allocate much)
crates/ty/src/lib.rs
Outdated
| if std::env::var("TY_MEMORY_REPORT").as_deref() == Ok("1") { | ||
| salsa_memory_dump(&db); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if:
- We would print a condenced memory report when running with
-vv - We would print the full memory report when running with
-vvv
You can get the verbosity from args.verbosity.
I would probably skip the environment variable for now. If we don't, make sure to add it here https://github.com/astral-sh/ty/blob/main/docs/reference/env.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having to run -vvv is a little difficult because of the amount of tracing logs you have to wait for :) I kept the environment variable but added short and full options. We might eventually have to add an option for mypy primer that keeps the diff less sensitive to minor changes.
007f92f to
23844f1
Compare
23844f1 to
c59855c
Compare
* main: [ty] Add builtins to completions derived from scope (#18982) [ty] Don't add incorrect subdiagnostic for unresolved reference (#18487) [ty] Simplify `KnownClass::check_call()` and `KnownFunction::check_call()` (#18981) [ty] Add micro-benchmark for #711 (#18979) [`flake8-annotations`] Make `ANN401` example error out-of-the-box (#18974) [`flake8-async`] Make `ASYNC110` example error out-of-the-box (#18975) [pandas]: Fix issue on `non pandas` dataframe `in-place` usage (PD002) (#18963) [`pylint`] Fix `PLC0415` example (#18970) [ty] Add environment variable to dump Salsa memory usage stats (#18928) [`pylint`] Fix `PLW0108` autofix introducing a syntax error when the lambda's body contains an assignment expression (#18678) Bump 0.12.1 (#18969) [`FastAPI`] Add fix safety section to `FAST002` (#18940) [ty] Add regression test for leading tab mis-alignment in diagnostic rendering (#18965) [ty] Resolve python environment in `Options::to_program_settings` (#18960) [`ruff`] Fix false positives and negatives in `RUF010` (#18690) [ty] Fix rendering of long lines that are indented with tabs [ty] Add regression test for diagnostic rendering panic [ty] Move venv and conda env discovery to `SearchPath::from_settings` (#18938)
## Summary Print the [new salsa memory usage dumps](#18928) in mypy primer CI runs to help us catch memory regressions. The numbers are rounded to the nearest power of 1.1 (about a 5% threshold between buckets) to avoid overly sensitive diffs.
Summary
Setting
TY_MEMORY_REPORT=fullwill generate and print a memory usage report to the CLI after aty checkrun:Eventually, we should integrate these numbers into CI in some form. The one limitation currently is that heap allocations in salsa structs (e.g. interned values) are not tracked, but memoized values should have full coverage. We may also want a peak memory usage counter (that accounts for non-salsa memory), but that is relatively simple to profile manually (e.g.
time -v ty check) and would require a compile-time option to avoid runtime overhead.Depends on salsa-rs/salsa#925.