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

Replace file locking logic with subdirectory #285

Closed
wants to merge 2 commits into from

Conversation

neysofu
Copy link

@neysofu neysofu commented Aug 19, 2024

Hi, thanks for trybuild! I use all the time, and it's great.

Sometimes, when I throw a lot of #[test]s at trybuild, I notice that they take a really long time. It seems that it's because they all run serially and not in parallel. I was thinking it might be better to create a separate directory per Runner and remove locking altogether, which would improve throughput.

@neysofu
Copy link
Author

neysofu commented Aug 19, 2024

I just realized that the target subdirectory will grow indefinitely with this change. I'll try to come up with a solution, and close the PR if I can't find any.

@neysofu neysofu marked this pull request as draft August 20, 2024 19:42
@neysofu neysofu marked this pull request as ready for review August 27, 2024 07:55
@neysofu
Copy link
Author

neysofu commented Aug 27, 2024

My new approach creates a temporary directory for each Project, removing the need for cargo clean as well.

Anecdotically, the tests on one of my projects take ~36s with this PR and ~89s without.

@dtolnay
Copy link
Owner

dtolnay commented Sep 6, 2024

Is the 36s project open source? I am interested to assess how problematic it might be for the output from concurrent tests to be interleaved.

@neysofu
Copy link
Author

neysofu commented Sep 6, 2024

Yes, you can take a look here: https://github.com/Sovereign-Labs/sovereign-sdk/tree/filippo/trybuild-repro.

The timings you'll get are different from the ones I gave you because I originally ran my benchmarks against a private version of the codebase which has significant changes. But you'll notice the tests still take longer without the patch regardless.

Steps to reproduce:

  1. cargo nextest run -p sov-modules-macros
  2. cargo nextest run -p sov-modules-macros (cached)
  3. Apply the trybuild patch at the root Cargo.toml
  4. cargo nextest run -p sov-modules-macros
  5. cargo nextest run -p sov-modules-macros (cached)

I get similar results with cargo test instead of cargo nextest.

@dtolnay
Copy link
Owner

dtolnay commented Sep 6, 2024

The vast majority of the time in your tests is being spent in the linker, because you are linking over 200 megabytes of dependencies into every individual test case.

What I would recommend is: stick to only compile_fail tests. Move your pass tests somewhere else and build them with Cargo as an integration test instead of trybuild. Then consolidate all the compile_fail tests into one TestCases. Trybuild will perform all the compilation in parallel, and will bypass linking. You should find that it is an order of magnitude faster than what you got with this PR.

@dtolnay dtolnay closed this Sep 6, 2024
@neysofu
Copy link
Author

neysofu commented Sep 6, 2024

Thanks for the great suggestions. Just wondering though, what's the problem with the temporary directory approach? Naively, I assumed it would make things better for other use cases as well compared to locking.

Also, putting all tests into one TestCases is possible but reduces our ability to run tests selectively – that's why I was hoping to make #[test] functions run in parallel altogether.

@dtolnay
Copy link
Owner

dtolnay commented Sep 6, 2024

For my own use cases, this doesn't end up being an improvement. I think the most number of UI test cases I have in one project is Serde which has 101 currently, and the compilation of all of them takes only 330ms. In my other projects it's even less. So I'd rather have the generated Cargo.toml around for troubleshooting, than what this PR does with temporary directories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants