Add local-only preprocessor cache AKA "direct mode" #1882

Alphare · 2023-09-11T15:49:16Z

sylvestre · 2023-10-11T08:22:41Z

@Alphare could you please write a trivial PR (typo fix, spaces, etc?) so that I don't have to approve the CI runs in the future? :)
thanks

Alphare · 2023-10-11T08:25:32Z

@Alphare could you please write a trivial PR (typo fix, spaces, etc?) so that I don't have to approve the CI runs in the future? :) thanks

Ah sorry, not used to working with Github too much, Mozilla's workflow even less. Will do quickly, thanks for the heads up. :)

codecov-commenter · 2023-10-11T08:33:12Z

Codecov Report

Attention: 821 lines in your changes are missing coverage. Please review.

Comparison is base (e911999) 29.70% compared to head (738ad92) 29.81%.

❗ Current head 738ad92 differs from pull request most recent head 194b7dd. Consider uploading reports for the commit 194b7dd to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1882      +/-   ##
==========================================
+ Coverage   29.70%   29.81%   +0.10%     
==========================================
  Files          50       51       +1     
  Lines       18022    19128    +1106     
  Branches     8703     9237     +534     
==========================================
+ Hits         5354     5703     +349     
- Misses       7491     7960     +469     
- Partials     5177     5465     +288

Files	Coverage Δ
src/compiler/diab.rs	`49.86% <0.00%> (-0.14%)`	⬇️
src/compiler/msvc.rs	`46.91% <0.00%> (-0.16%)`	⬇️
src/compiler/nvcc.rs	`41.02% <0.00%> (-0.16%)`	⬇️
src/compiler/nvhpc.rs	`33.83% <0.00%> (-0.26%)`	⬇️
src/compiler/tasking_vx.rs	`43.52% <0.00%> (-0.12%)`	⬇️
src/compiler/clang.rs	`53.74% <25.00%> (-0.22%)`	⬇️
src/test/tests.rs	`34.86% <50.00%> (+1.53%)`	⬆️
src/cmdline.rs	`15.54% <0.00%> (-0.44%)`	⬇️
src/compiler/rust.rs	`32.07% <33.33%> (-0.09%)`	⬇️
src/config.rs	`28.28% <16.66%> (-1.70%)`	⬇️
... and 8 more

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Alphare · 2023-10-11T16:45:49Z

Aside from a couple of grunt-work TODOs, this should be good enough to review. I'd love to get some feedback while I address those + add the user-level docs so that we don't get too much round-trip lag. :)

src/compiler/c.rs

sylvestre · 2023-10-12T19:22:05Z

src/config.rs

+    };
+
+    if env::var("SCCACHE_DIRECT").is_ok() && env::var("SCCACHE_NODIRECT").is_ok() {
+        warn!("Both `SCCACHE_DIRECT` && `SCCACHE_NODIRECT` are defined");


Suggested change

warn!("Both `SCCACHE_DIRECT` && `SCCACHE_NODIRECT` are defined");

warn!("Conflicting options: Both `SCCACHE_DIRECT` && `SCCACHE_NODIRECT` are defined");

Imho we should prefer a single env var name and use its value.

That works for me. I don't think accepted boolean values are unified across sccache. Usually (1, on, true, 0, off, false), but maybe we want to be stricter (and complain when we don't get a valid value)?

I think there was a clap extension / or impl. to handle the different variants which should be used for all of these features long term, but that'd be a breaking change. Adding new features / envs being interpreted should assume that uniform version https://docs.rs/clap/latest/clap/builder/struct.BoolishValueParser.html , at least that would simplify behavior.

sylvestre · 2023-10-12T19:23:03Z

tests/test.c.gcc-13.2.0-preproc

@@ -0,0 +1,743 @@
+# 0 "tests/test.c"


would it be possible to have a clang preproc file too ?

For this particular test, preprocessor output was identical. I'm not exactly sure how complex the test would need to grow to get a different preprocessor output. Does anyone have an example? I'll try to look into it if not.

Xuanwo · 2023-10-13T03:42:07Z

Is it possible to make direct mode work of storage backend like s3 and gcs?

docs/Configuration.md

docs/Local.md

src/cache/cache.rs

src/compiler/manifest.rs

src/util.rs

drahnr

The code quality and documentation is very good! There is a range of minor nits that should be addressed before merging.
Thank you for your PR!

glandium · 2023-10-24T06:08:18Z

src/util.rs

+    /// plus MAX_HAYSTACK_LEN bytes of the previous chunk, to account for
+    /// the possibility of partial reads splitting a time macro
+    /// across two calls.
+    previous_small_read: Option<Vec<u8>>,


These changes should go in the previous commit rather than this one. I wonder if it wouldn't be simpler to use a single buffer instead of overlap_buffer + previous_small_read. (I'll also note an Option<Vec<T>> is kind of pointless if you don't expect there to be a Some(vec![]).

These changes should go in the previous commit rather than this one.

Good catch.

I wonder if it wouldn't be simpler to use a single buffer instead of overlap_buffer + previous_small_read.

IMO it would make the logic harder to read.

(I'll also note an Option<Vec<T>> is kind of pointless if you don't expect there to be a Some(vec![]).

True. While it doesn't matter in terms of performance, I'll switch to just a Vec<u8>, should make the code simpler.

IMO it would make the logic harder to read.

I'm not sure it would. I'm having a hard time convincing myself that the current one is correct with all the dancing around between both buffers.

glandium · 2023-10-24T06:09:47Z

src/cache/disk.rs

@@ -173,7 +173,7 @@ impl Storage for DiskCache {
    ) -> Result<()> {
        let key = normalize_key(key);
        let mut buf = vec![];
-        preprocessor_cache_entry.write(&mut buf)?;
+        preprocessor_cache_entry.serialize_to(&mut buf)?;


It's weird that you do a mix of amends and additional commits to fixup things.

Yeah, sorry, I usually always to amends, but I was starting to take too much time to do history editing. I should probably stick to new commits in this case, unless otherwise asked (like with the time macros thing above).

glandium · 2023-10-24T06:26:07Z

src/compiler/gcc.rs

@@ -435,6 +437,11 @@ where
            },
        };

+        too_hard_for_direct_mode = match arg.flag_str() {
+            Some(s) => s == "-Xpreprocessor",


We should probably add -Wp in ARGS. Probably something like take_arg!("-Wp", OsString, Concatenated(','), PreprocessorArgument)

I guess you could put this check for -Xpreprocessor/-Wp where ProcessorArgument are handled, rather than for all args. Then there's the question about the second loop for Xclangs, and, how @-files are handled with direct mode.

glandium · 2023-10-24T06:31:11Z

src/compiler/preprocessor_cache.rs

@@ -560,7 +560,7 @@ mod test {
        let mut buf = vec![0; HASH_BUFFER_SIZE * 2];
        // Make the pattern overlap two buffer chunks to make sure we account for this
        let start = HASH_BUFFER_SIZE - MAX_TIME_MACRO_HAYSTACK_LEN / 2;
-        buf[start..start + b"__TIMESTAMP__".len()].copy_from_slice(b"__TIMESTAMP__");
+        buf[start..][..b"__TIMESTAMP__".len()].copy_from_slice(b"__TIMESTAMP__");


Could me amended into the commit where you add these tests.

glandium · 2023-10-24T06:32:34Z

src/compiler/c.rs

@@ -884,7 +884,7 @@ fn remember_include_file(

    let original_path = path;
    // Canonicalize path for comparison; Clang uses ./header.h.
-    if path.starts_with(b"./") {
+    if path.starts_with(b"./") || path.starts_with(br".\") {


In theory a ".\foo" file could literally exist on Unix, and that would break in that case.

I'll special case this for Windows, thanks.

Alphare · 2023-10-24T08:39:45Z

(For some reason Github doesn't want me replying to this comment, so I'll do it here)

We should probably add -Wp in ARGS. Probably something like take_arg!("-Wp", OsString, Concatenated(','), PreprocessorArgument)

I guess you could put this check for -Xpreprocessor/-Wp where ProcessorArgument are handled, rather than for all args.

Sounds good

Then there's the question about the second loop for Xclangs,

Looking at ccache's code, Xclangs are only handled for PCH stuff that we don't cover in sccache. Nothing that implies direct mode anyway.

and, how @-files are handled with direct mode.

IIUC, sccache expands @-file arguments at the start, so it shouldn't make any difference?

Alphare · 2023-10-24T12:15:21Z

This last push should fix the clippy issue. I think I've addressed your comments.

glandium · 2023-10-26T22:39:44Z

src/compiler/c.rs

+    // Canonicalize path for comparison; Clang uses ./header.h.
+    #[cfg(windows)]
+    {
+        if path.starts_with(br".\") {


The fun thing about windows is that it can also use forward slash as a path separator.

glandium · 2023-10-26T22:42:16Z

src/compiler/gcc.rs

-            | Some(PreprocessorArgumentPath(_)) => &mut preprocessor_args,
+            | Some(PreprocessorArgumentPath(_)) => {
+                too_hard_for_preprocessor_cache_mode = match arg.flag_str() {
+                    Some(s) => s == "-Xpreprocessor" || s == "-Wp",


Not that it would make a huge difference, but both of these are PreprocessorArgument, you could put the check behind PreprocessorArgument rather than all the PreprocessorArgument*.

glandium · 2023-10-26T22:43:51Z

src/compiler/gcc.rs

@@ -2054,5 +2056,12 @@ mod test {
            o => panic!("Got unexpected parse result: {:?}", o),
        };
        assert!(parsed_args.too_hard_for_preprocessor_cache_mode);
+
+        let args = stringvec!["-c", "foo.c", "-o", "foo.o", r#"-Wp,-DFOO="'something'""#];


You can remove the single quotes.

glandium · 2023-10-26T22:57:24Z

src/util.rs

+    /// plus MAX_HAYSTACK_LEN bytes of the previous chunk, to account for
+    /// the possibility of partial reads splitting a time macro
+    /// across two calls.
+    previous_small_read: Option<Vec<u8>>,


IMO it would make the logic harder to read.

I'm not sure it would. I'm having a hard time convincing myself that the current one is correct with all the dancing around between both buffers.

glandium · 2023-10-26T22:59:53Z

src/util.rs

+                } else {
+                    self.previous_small_read = visit.to_owned();
+                }
+                self.find_macros(&self.previous_small_read.to_owned());


Useless to_owned?

glandium · 2023-10-26T23:00:55Z

src/util.rs

+                    .copy_from_slice(visit);
+
+                // Check both the concatenation with the previous small read
+                self.find_macros(&self.previous_small_read.to_owned());


Useless to_owned

glandium · 2023-10-26T23:01:03Z

src/util.rs

+                // Check both the concatenation with the previous small read
+                self.find_macros(&self.previous_small_read.to_owned());
+                // ...and the overlap buffer
+                self.find_macros(&self.overlap_buffer.clone());


Useless clone

glandium · 2023-10-26T23:01:13Z

src/util.rs

+                let right_half = visit.len() - MAX_HAYSTACK_LEN;
+                self.overlap_buffer[..MAX_HAYSTACK_LEN].copy_from_slice(&visit[right_half..]);
+            }
+            self.find_macros(&self.overlap_buffer.clone());


Useless clone

glandium · 2023-10-26T23:01:21Z

src/util.rs

+                // Copy the left side of the visit to the right of the buffer
+                let left_half = MAX_HAYSTACK_LEN;
+                self.overlap_buffer[left_half..].copy_from_slice(&visit[..left_half]);
+                self.find_macros(&self.overlap_buffer.clone());


Useless clone

sylvestre · 2023-10-29T08:11:44Z

Build failed here:

   Compiling sccache v0.6.0 (/home/runner/work/sccache/sccache)
error[E0433]: failed to resolve: use of undeclared crate or module `io`
   --> src/compiler/c.rs:842:51
    |
842 |     fn metadata(&self, path: impl AsRef<Path>) -> io::Result<PreprocessorFileMetadata> {
    |                                                   ^^ use of undeclared crate or module `io`
    |
help: a builtin type with a similar name exists
    |
842 |     fn metadata(&self, path: impl AsRef<Path>) -> i8::Result<PreprocessorFileMetadata> {
    |                                                   ~~
help: consider importing one of these items
    |
15  + use futures::io;
    |
15  + use std::io;
    |
15  + use tokio::io;
    |

This will be useful in a later commit to persist file paths using the same binary format on all platforms. This also introduces a decoding operation, which adapts the code from the original library ever so slightly to be less likely to provoke UB from a future change. Instead of: ``` // Convert to UTF-16 let mut wstr: Vec<u16> = Vec::with_capacity(len as usize); wstr.set_len(len as usize); let len = winapi::um::stringapiset::MultiByteToWideChar( codepage, flags, multi_byte_str.as_ptr() as winapi::um::winnt::LPSTR, multi_byte_str.len() as i32, wstr.as_mut_ptr(), len, ); ``` Do: ``` // Convert to UTF-16 let mut wstr: Vec<u16> = Vec::with_capacity(len as usize); let len = winapi::um::stringapiset::MultiByteToWideChar( codepage, flags, multi_byte_str.as_ptr() as winapi::um::winnt::LPSTR, multi_byte_str.len() as i32, wstr.as_mut_ptr(), len, ); wstr.set_len(len as usize); Which moves the `set_len` to *after* the Vec has been initialized. ```

This will be useful in a future commit to compare timestamps with as much precision as possible while also handling obviously fake/corrupted times.

This will be useful to handle time macros properly during caching preprocessor output in a future commit, while also not reading the contents twice.

`ccache` has a direct mode¹ by default, which caches the preprocessor step whenever possible, which speeds up local computation considerably. This only applies to C/C++ compilers and local storage. [1] https://ccache.dev/manual/4.3.html#_the_direct_mode

Useful for debugging.

This makes testing much easier and the structure of the overall function clearer, at the cost of some added complexity.

This stops using a dummy `TimeMacroFinder` and just separates the logic between the two modes.

This is not technically required to work, but will make our cache more reproducible.

It's more idiomatic in the Rust world, and makes it more obvious.

We define the `-Wp,*` argument for GCC/Clang, disable direct mode if it's present, since it's too hard to handle. `ccache` allows `-Wp,-MD,path`, `-Wp,-MMD,path` and `-Wp,-D_define_`. We don't handle these for now, since they need more careful attention.

sylvestre · 2023-10-30T15:41:07Z

well done @Alphare!
We can fix the rest in new pr

sylvestre · 2023-10-30T16:00:13Z

@robertmaynard if you have a chance to test it before the release, don't hesitate

sylvestre · 2023-11-04T21:13:31Z

docs/Configuration.md

@@ -84,6 +99,7 @@ configuration variables

 * `SCCACHE_DIR` local on disk artifact cache directory
 * `SCCACHE_CACHE_SIZE` maximum size of the local on disk cache i.e. `2G` - default is 10G
+* `SCCACHE_PREPROCESSOR_MODE` enable/disable preprocessor caching (see [the local doc](Local.md))


it should be "SCCACHE_DIRECT" no ?

Alphare force-pushed the direct-mode branch from 6ffa7b9 to 8832194 Compare October 11, 2023 08:09

Alphare force-pushed the direct-mode branch 5 times, most recently from f015f48 to 8b0671b Compare October 11, 2023 16:28

Alphare changed the title ~~WIP add local-only direct mode~~ Add local-only direct mode Oct 11, 2023

Alphare marked this pull request as ready for review October 11, 2023 16:46

sylvestre requested review from Xuanwo, glandium and drahnr October 11, 2023 18:16

sylvestre reviewed Oct 12, 2023

View reviewed changes

src/compiler/c.rs Show resolved Hide resolved

sylvestre reviewed Oct 12, 2023

View reviewed changes

src/compiler/c.rs Outdated Show resolved Hide resolved

sylvestre reviewed Oct 12, 2023

View reviewed changes