chore: Explicitly depend on xyz_derive crates #5637
Conversation
|
Before: Run 2 After: Run 1 Run 2 |
| @@ -18,10 +18,10 @@ use milli::Index; | |||
| use serde_json::Value; | |||
| use tempfile::TempDir; | |||
|
|
|||
| #[derive(Debug, Arbitrary)] | |||
| #[derive(Debug, derive_arbitrary::Arbitrary)] | |||
There was a problem hiding this comment.
Is this really useful?
We're building the derive crate only once so we can use the derive version or the re-exported version the same and it should work right? 🤔
There was a problem hiding this comment.
Here is how it breaks with the old/current code:
error[E0433]: failed to resolve: could not find `Arbitrary` in `arbitrary`
--> crates/fuzzers/src/bin/fuzz-indexing.rs:21:28
|
21 | #[derive(Debug, arbitrary::Arbitrary)]
| ^^^^^^^^^ could not find `Arbitrary` in `arbitrary`
error[E0599]: the function or associated item `arbitrary` exists for array `[Batch; 5]`, but its trait bounds were not satisfied
--> crates/fuzzers/src/bin/fuzz-indexing.rs:79:49
|
22 | struct Batch([Operation; 5]);
| ------------ doesn't satisfy `Batch: Arbitrary<'_>`
...
79 | let batches = <[Batch; 5]>::arbitrary(&mut data).unwrap();
| ^^^^^^^^^ function or associated item cannot be called on `[Batch; 5]` due to unsatisfied trait bounds
|
= note: the following trait bounds were not satisfied:
`Batch: Arbitrary<'_>`
which is required by `[Batch; 5]: Arbitrary<'_>`
note: the trait `Arbitrary` must be implemented
--> /home/martin/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arbitrary-1.4.1/src/lib.rs:163:1
|
163 | pub trait Arbitrary<'a>: Sized {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Since the derive feature is not enabled for the arbitrary crate some of its exports are not reachable:
There was a problem hiding this comment.
Aaah, yes, my bad, I didn't notice you also removed the feature.
Then, an additional question: shouldn't we be able to keep the derive feature in arbitrary and still get the compile time improvement because cargo can start compiling it before it takes on arbitrary (and then when arbitrary arrives, if it's already compiled, then there is nothing to do)?
There was a problem hiding this comment.
This should work too, I think!
Let me try it!
There was a problem hiding this comment.
Unfortunately this way the benefit is gone.
Run 1:
________________________________________________________
Executed in 330.66 secs fish external
usr time 15.33 mins 1.15 millis 15.33 mins
sys time 1.47 mins 0.04 millis 1.47 mins
Run 2:
________________________________________________________
Executed in 332.93 secs fish external
usr time 15.46 mins 1.25 millis 15.46 mins
sys time 1.49 mins 0.07 millis 1.49 mins
I have pushed the change for review but I think it should be reverted if this PR is going to be merged.
This improves the build time by building both xyz and xyz_derive crates in parallel. By using a feature the `xyz`'s build will need to wait for `xyz_derive`'s build. Suggested-at: serde-rs/serde#2907 (comment) Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
The idea to trigger the build of xyz_derive crate in parallel with the main crate. Unfortunately it does not bring the build time benefit. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Fixing: ``` error: the lock file /__w/meilisearch/meilisearch/Cargo.lock needs to be updated but --locked was passed to prevent this If you want to try to generate the lock file without accessing the network, remove the --locked flag and use --offline instead. ``` Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g
force-pushed
the
compile-serde-derive-in-parallel
branch
from
c6c308f to
55f580a
Compare
|
|
martin-g
changed the title
Pull Request
Related issue
N/A
What does this PR do?
This improves the full build time by building both xyz and xyz_derive crates in parallel.
By using a feature the
xyz's build will need to wait forxyz_derive's build.The incremental build times should not be affected.
Suggested-at: serde-rs/serde#2907 (comment)