Open cache files in parallel by Thomasdezeeuw · Pull Request #5120 · astral-sh/ruff

Thomasdezeeuw · 2023-06-15T13:35:49Z

Summary

This is based on #5117.

Open cache files in parallel (again).

Test Plan

Existing tests should keep working.

github-actions · 2023-06-15T13:49:05Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.1±0.30ms     5.8 MB/sec    1.01      7.1±0.29ms     5.7 MB/sec
formatter/numpy/ctypeslib.py               1.01  1499.9±79.92µs    11.1 MB/sec    1.00  1484.0±67.92µs    11.2 MB/sec
formatter/numpy/globals.py                 1.00   149.8±10.72µs    19.7 MB/sec    1.04   155.9±15.25µs    18.9 MB/sec
formatter/pydantic/types.py                1.00      3.0±0.15ms     8.6 MB/sec    1.02      3.0±0.19ms     8.4 MB/sec
linter/all-rules/large/dataset.py          1.01     14.7±0.48ms     2.8 MB/sec    1.00     14.6±0.50ms     2.8 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.7±0.14ms     4.5 MB/sec    1.01      3.7±0.21ms     4.5 MB/sec
linter/all-rules/numpy/globals.py          1.02   497.9±30.36µs     5.9 MB/sec    1.00   487.2±22.89µs     6.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.5±0.25ms     3.9 MB/sec    1.01      6.6±0.23ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.00      7.4±0.31ms     5.5 MB/sec    1.01      7.5±0.44ms     5.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02  1639.6±84.02µs    10.2 MB/sec    1.00  1608.2±90.52µs    10.4 MB/sec
linter/default-rules/numpy/globals.py      1.00   190.9±10.24µs    15.5 MB/sec    1.02   194.6±14.40µs    15.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.4±0.13ms     7.5 MB/sec    1.00      3.4±0.15ms     7.5 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.13     11.5±0.36ms     3.6 MB/sec    1.00     10.2±0.32ms     4.0 MB/sec
formatter/numpy/ctypeslib.py               1.16      2.3±0.09ms     7.1 MB/sec    1.00      2.0±0.07ms     8.3 MB/sec
formatter/numpy/globals.py                 1.14   238.8±10.23µs    12.4 MB/sec    1.00   209.8±11.26µs    14.1 MB/sec
formatter/pydantic/types.py                1.16      4.9±0.18ms     5.2 MB/sec    1.00      4.3±0.18ms     6.0 MB/sec
linter/all-rules/large/dataset.py          1.04     19.8±0.61ms     2.1 MB/sec    1.00     19.1±0.71ms     2.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      5.2±0.13ms     3.2 MB/sec    1.04      5.4±0.26ms     3.1 MB/sec
linter/all-rules/numpy/globals.py          1.00   666.7±27.83µs     4.4 MB/sec    1.01   676.0±58.82µs     4.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      8.9±0.28ms     2.9 MB/sec    1.03      9.2±0.28ms     2.8 MB/sec
linter/default-rules/large/dataset.py      1.00     10.5±0.36ms     3.9 MB/sec    1.00     10.6±0.41ms     3.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.03      2.3±0.10ms     7.3 MB/sec    1.00      2.2±0.06ms     7.5 MB/sec
linter/default-rules/numpy/globals.py      1.04   276.8±11.48µs    10.7 MB/sec    1.00   264.9±21.50µs    11.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.9±0.18ms     5.2 MB/sec    1.00      4.8±0.22ms     5.3 MB/sec

MichaReiser · 2023-06-19T15:28:56Z

Do I understand this change correctly that it mainly opens the cache "lazily" and it's our parallel traversal of the files that make the cache loading parallel?

Thomasdezeeuw · 2023-06-19T15:53:27Z

Do I understand this change correctly that it mainly opens the cache "lazily" and it's our parallel traversal of the files that make the cache loading parallel?

Initially this was true. But with the last commit I've simplified this because I didn't really like the code. Now the caches are opened in parallel, but no longer lazily. This allows us to avoid atomic/locks in reading from the cache (to synchronize the reading of the cache).

MichaReiser · 2023-06-20T06:58:02Z

crates/ruff_cli/src/cache.rs

+    fn same_results() {
+        let mut cache_dir = temp_dir();
+        cache_dir.push("ruff_tests/cache_same_results");
+        let _ = fs::remove_dir_all(&cache_dir);


Have you considered using https://crates.io/crates/tempdir to ensure we always clean up the temp directory, including when the test succeeds?

I usually try to minimize the dependencies used, but I'm perfectly happy switching to it.

MichaReiser · 2023-06-20T06:59:44Z

crates/ruff_cli/src/cache.rs

+                expected_diagnostics += lint_path(
+                    &path,
+                    Some(&package_root),
+                    &settings,
+                    Some(&cache),
+                    flags::Noqa::Enabled,
+                    flags::FixMode::Generate,
+                )
+                .unwrap();


This is a fairly heavy integration test that can break for many reasons. Would it make sense to:

use a smaller fixture directory

Test the cache in isolation rather than depending on lint_path (calling the cache API directly)

I understand this test is a bit heavy compared to some of the other tests in the ruff_cli crate, but I would be hesitant to make it any weaker. This is not only testing the caching stuff it's also testing the recreation of the diagnostics, which will be more complex if we switch to storing only a partial source for example.

So I rather keep this integration test, but perhaps we can move it somewhere else (which would mean we need to make some of these functions public).

crates/ruff_cli/src/cache.rs

MichaReiser · 2023-06-20T07:03:21Z

crates/ruff_cli/src/cache.rs

+        let mut cache_dir = temp_dir();
+        cache_dir.push("ruff_tests/cache_invalidation");
+        let _ = fs::remove_dir_all(&cache_dir);
+        cache::init(&cache_dir).unwrap();
+
+        let settings = AllSettings::default();
+        let package_root = fs::canonicalize(&cache_dir).unwrap();
+        let cache = Cache::open(&cache_dir, package_root.to_owned(), &settings.lib);
+        assert_eq!(cache.new_files.lock().unwrap().len(), 0);


Nit: It could make sense to move some of the shared setup code into a helper function that returns the created Cache or a wrapper object. This can help with readability

Example:

ruff/crates/ruff_python_formatter/src/comments/mod.rs

Lines 434 to 438 in 4cc3cdb

let test_case = CommentsTestCase::from_code(source);

let comments = test_case.to_comments();

assert_debug_snapshot!(comments.debug(test_case.source_code));

Agreed, I didn't bother with it because it's just four calls to Cache::open. Do you mind if I leave it for now?

crates/ruff_cli/src/cache.rs

MichaReiser · 2023-06-20T07:07:29Z

crates/ruff_cli/src/cache.rs

+pub(crate) type RelativePath = Path;
+/// [`PathBuf`] that is relative to the package root in [`PackageCache`].
+pub(crate) type RelativePathBuf = PathBuf;


Nit: Would it make sense to create newtype wrappers instead that implement Deref to enforce the difference statically?

We could I'm not sure it it's worth the maintenance burden though.

crates/ruff_cli/src/cache.rs

Instead log a warning and move on without a cache.

This a little complex so see the comment in the code for an explanation.

This adds an atomic boolean used to keep track to changes to the PackageCache and if not triggered it will not write the cache file back to disk as it will be the same as the already on disk file.

Cache will be the type managing the cache (file), while PackageCache now only holds the on-disk data. This changes the way how the cache is created and updated. The new Cache type keeps track of the source file (and cache) that changed, meaning that if the source file didn't change we don't have to do much (other than printing the diagnostics to the user). This means we can remove the lock from PackageCache::files, making reading the cache files cheaper. Of course we still need a lock around Cache::new_files to keep track of the files that have changed. But working with the assumption that changed source file are smaller in number than the unchanged source files this leads to less contention on the hashmap lock (that now has been reduced in scope). This, along side the previous improvements, reduce a fully cached run to be ~10% slower (up from 50%+ slower) than the old cache implementation, that used a cache file per source file resulting in larger cache sizes.

For source files that are not part of a Python package (as currently resolved) fall back to using the directory name instead of the file name as package root. This reduces the amount of cache file created. Some examples: Repo | Old | New Bokeh | 744 | 120 CPython | 361 | 101 FastAPI | 518 | 79 Jupyter Server | 23 | 13

This doesn't really impact the performance much, but the code is much simpler.

This uses all test file in crates/ruff/resources/test/fixtures.

Thomasdezeeuw · 2023-06-20T10:18:13Z

I'm getting an error on the CI that the jupyter/R.ipynb is missing from the cache, but I can't reproduce it locally...

crates/ruff_cli/src/cache.rs

Seems to fail on the CI, but it's hard to produce locally (for me).

instead of using the temporary file system add a test fixture.

Thomasdezeeuw requested review from charliermarsh and konstin June 15, 2023 13:35

Thomasdezeeuw mentioned this pull request Jun 16, 2023

Only use a single cache file per Python package #5117

Merged

Thomasdezeeuw force-pushed the thomas/open_cache_in_parallel branch 2 times, most recently from e2f5bd1 to 3d06953 Compare June 16, 2023 09:21

Thomasdezeeuw requested a review from MichaReiser June 19, 2023 09:26

Thomasdezeeuw force-pushed the thomas/cache branch from 8c3fa7a to d96d998 Compare June 19, 2023 14:24

Thomasdezeeuw force-pushed the thomas/open_cache_in_parallel branch from 83d4475 to 97221c8 Compare June 19, 2023 14:26

Thomasdezeeuw marked this pull request as ready for review June 19, 2023 14:26

Base automatically changed from thomas/cache to main June 19, 2023 15:46

Thomasdezeeuw force-pushed the thomas/open_cache_in_parallel branch from 97221c8 to c128b64 Compare June 19, 2023 15:50

MichaReiser approved these changes Jun 20, 2023

View reviewed changes

Thomas de Zeeuw added 12 commits June 20, 2023 12:17

Don't fail when the cache can't be read

befaf25

Instead log a warning and move on without a cache.

Open cache files in parallel

3292e13

This a little complex so see the comment in the code for an explanation.

Store cache files in parallel

b87b5ae

Don't write cache file if they haven't changed

4870e2d

This adds an atomic boolean used to keep track to changes to the PackageCache and if not triggered it will not write the cache file back to disk as it will be the same as the already on disk file.

Fix Clippy warnings

f82d1df

Simplify loading of caches

8645c7f

This doesn't really impact the performance much, but the code is much simpler.

Test caching

f5cbabe

This uses all test file in crates/ruff/resources/test/fixtures.

Add cache invalidation test

4113794

Improve Cache docs

68723cc

Stricter cached files checking

4770146

Sort parse errors paths

aa1c7fa

Thomasdezeeuw force-pushed the thomas/open_cache_in_parallel branch from 42f2bf1 to aa1c7fa Compare June 20, 2023 10:27

MichaReiser reviewed Jun 20, 2023

View reviewed changes

crates/ruff_cli/src/cache.rs Outdated Show resolved Hide resolved

Thomas de Zeeuw added 4 commits June 20, 2023 14:52

Ignore R.ipynb file in cache test

35baba1

Seems to fail on the CI, but it's hard to produce locally (for me).

Avoid thread::sleep in test

254138c

instead of using the temporary file system add a test fixture.

Use assert_{eq,ne} where possible

abc1202

Ignore ruff_cli resources

8aa78f2

konstin closed this Jun 20, 2023

konstin reopened this Jun 20, 2023

Skip black_compatibility_test

2f48ce9

Thomasdezeeuw mentioned this pull request Jun 20, 2023

Renable black compatibility test #5208

Closed

konstin removed their request for review June 20, 2023 15:21

Thomasdezeeuw merged commit 17f1ecd into main Jun 20, 2023

Thomasdezeeuw deleted the thomas/open_cache_in_parallel branch June 20, 2023 15:43

chenrui333 mentioned this pull request Jun 20, 2023

ruff 0.0.273 Homebrew/homebrew-core#134306

Merged

hauntsaninja mentioned this pull request Jun 20, 2023

[Panic] in pre-commit with v0.0.273 #5225

Closed

Thomasdezeeuw mentioned this pull request Jun 27, 2023

🐢 .ruff_cache directory has a huge size and it slows down PyCharm load #5132

Closed

	let test_case = CommentsTestCase::from_code(source);

	let comments = test_case.to_comments();

	assert_debug_snapshot!(comments.debug(test_case.source_code));

Conversation

Thomasdezeeuw commented Jun 15, 2023

Summary

Test Plan

Uh oh!

github-actions bot commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

MichaReiser commented Jun 19, 2023

Uh oh!

Thomasdezeeuw commented Jun 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Thomasdezeeuw commented Jun 20, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jun 15, 2023 •

edited

Loading