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

[red-knot] fix incremental benchmark #12400

Merged
merged 2 commits into from
Jul 19, 2024
Merged

[red-knot] fix incremental benchmark #12400

merged 2 commits into from
Jul 19, 2024

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Jul 19, 2024

Was it intentional that we rewrite foo.py with the BAR_CODE in the incremental benchmark?

@carljm carljm added the red-knot Multi-file analysis & type inference label Jul 19, 2024
@carljm carljm requested a review from MichaReiser July 19, 2024 00:34
Copy link
Contributor

github-actions bot commented Jul 19, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@@ -138,7 +138,7 @@ fn benchmark_incremental(criterion: &mut Criterion) {
case.fs
.write_file(
SystemPath::new("/src/foo.py"),
format!("{BAR_CODE}\n# A comment\n"),
format!("{FOO_CODE}\n# A comment\n"),
Copy link
Member

Choose a reason for hiding this comment

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

I think the real error here is that we write /src/foo instead of bar. Because we then touch bar and not foo.

The idea is to invalidate a dependency and then re-run checking which should show how well we can cope with local changes.

Base automatically changed from cjm/fix-benchmark-naming to cjm/bench-preparse-builtins July 19, 2024 05:54
Base automatically changed from cjm/bench-preparse-builtins to main July 19, 2024 05:58
@carljm carljm force-pushed the cjm/fix-incremental-bench branch from ac32227 to 5afe2a2 Compare July 19, 2024 14:59
Copy link

codspeed-hq bot commented Jul 19, 2024

CodSpeed Performance Report

Merging #12400 will degrade performances by 55.21%

Comparing cjm/fix-incremental-bench (5afe2a2) with main (f82bb67)

Summary

❌ 1 (👁 1) regressions
✅ 32 untouched benchmarks

Benchmarks breakdown

Benchmark main cjm/fix-incremental-bench Change
👁 red_knot_check_file[incremental] 94.8 µs 211.7 µs -55.21%

@carljm
Copy link
Contributor Author

carljm commented Jul 19, 2024

This change of course makes the incremental benchmark slower, since we now change the dependency and impact both files, requiring more validation of dependencies. It's still faster than the non-incremental benchmark though, which is good; the results of type inference on bar don't change due to the added comment and we can avoid redoing some work in foo.

@carljm carljm merged commit 1c7b840 into main Jul 19, 2024
20 checks passed
@carljm carljm deleted the cjm/fix-incremental-bench branch July 19, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants