-
Notifications
You must be signed in to change notification settings - Fork 12.1k
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
Greatly speed up doctests by compiling compatible doctests in one file #123974
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
bff1867
to
2188a37
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
48e04f4
to
4c766eb
Compare
Seems like where there are enough tests, |
This comment has been minimized.
This comment has been minimized.
7796f3f
to
df03145
Compare
This comment has been minimized.
This comment has been minimized.
df03145
to
b4b72ca
Compare
This comment has been minimized.
This comment has been minimized.
b4b72ca
to
76c9ae6
Compare
This comment has been minimized.
This comment has been minimized.
Some changes occurred in run-make tests. cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
There should probably be some opt out from this merging. There are numerous reasons why two pieces of code may cause issues when combined together (they should compete for some common resource, in general, and that is not always detectable by a compiler). |
How does this affect env_logger? |
c7e80be
to
b3261d5
Compare
It fails in multiple tests:
I implemented a fallback in case compilation fails. I could do the same for failing tests. Like that, we can support both current doctests with shared context and others who don't have have this shared context issue can enjoy having doctests running much faster. What do you think? EDIT: Another solution would be to add a new codeblock attribute like "standalone" to say that a doctest should not be combined with others, but I'm not a big fan of this approach. |
That only helps with spurious failure. Spurious success is an even worse problem, if a test case accidentally relies on a global resource that another test set up. Any kind of merging has this problem, but it gets worse if the merging only happens sometimes. This is also a problem with |
The logic currently is as follows: we merge tests, if the compilation of the combined tests failed, we fallback to the current behaviour. One thing I thought though: we could spawn a new process for each test. That would prevent them from using "in program" common resources.
Yes it's the current issue with |
The troublesome possibility that I'm thinking of is:
|
What about changing the default way to run doctests starting the 2024 edition? EDIT: With a new |
That's better than implicit fallbacks. |
Agreed. Should I add the new |
…pdate associated test to check both combined and non-combined doctests
To do so, AST error detection was improved in order to not filter out too many doctests.
9f1abb4
to
2e3c2aa
Compare
Fixed merge conflict. |
This seems like a good idea in general, but the example given with |
If we do consider the static-detection approach, it could potentially be implemented by creating a MIR query |
I think it'd be a nice improvement but I don't think it should be a part of this first iteration as I'm pretty sure there are cases where using statics in this context is perfectly fine and we'd need to be go through a lot of code to check in which cases it's ok or not. |
Fair point. I guess this change is reasonable because it matches the way normal unit tests work (all compiled into one binary). Should we consider running all compatible doctests in a single binary in pre-2024 editions, then run those tests individually too, and check if the result didn't match the results of the individual tests (i.e., combined failed but all individual passed, or combined passed but at least one individual failed)? The challenge of course is that it'd cause a further slowdown on pre-2024 tests. Just a thought though to help migration to the new edition behavior. Also curious what some other people on the team think about this change in behavior. |
I'm not sure to understand what you have in mind in this case. Is it to uncover bugs we might have missed maybe?
That's why we'll have an FCP in any case. :) |
No, more to detect problems with user code. E.g. if doctests in someone's crate do something like depend on the same mutable global state, we can warn them that this will break in Rust 2024. |
I see. I'm not sure it's a good idea though. Maybe adding an option to force this new behaviour on non-2024 edition runs could be a way for users to give it a try? |
Yeah, we should probably focus on that for now. Could also consider a crater run. |
I'm still hoping we can make it before the 2024 edition is done. Let's hope I'm not too optimistic here. ^^' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments about the docs. In terms of the code itself, I believe we should refactor the doctest code before making this change. It's hard to follow what's happening since test collection, processing, and running are all mixed together—not because of this PR, rather due to the existing code. There are also other issues like functions with tons of arguments, which is improved somewhat here but should ideally be done in a separate PR that doesn't change behavior.
I'm currently working on such a refactor and will open a PR soon. Just to be clear, my goal is not to overhaul the test running system but just try to organize and decouple the code better.
@@ -376,6 +376,58 @@ that the code sample should be compiled using the respective edition of Rust. | |||
# fn foo() {} | |||
``` | |||
Starting the 2024 edition[^edition-note], compatible doctests will be merged as one before being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting the 2024 edition[^edition-note], compatible doctests will be merged as one before being | |
Starting in the 2024 edition[^edition-note], compatible doctests will be merged as one before being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps also add a sentence explaining that it's for performance reasons? Just so people don't wonder why we're making the change. Could also be worth clarifying that if it fails, the individual tests will be rerun to give an exact error.
@@ -376,6 +376,58 @@ that the code sample should be compiled using the respective edition of Rust. | |||
# fn foo() {} | |||
``` | |||
Starting the 2024 edition[^edition-note], compatible doctests will be merged as one before being | |||
run. It means that they will share the process, so any change global/static variables will now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run. It means that they will share the process, so any change global/static variables will now | |
run. It means that they will share the process, so any change to global/static variables will now |
.arg("--edition") | ||
.arg(edition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: add an edition()
helper to Rustdoc
just like the one on Rustc
.
.arg("--extern") | ||
.arg(format!("foo={}", dep.display())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: add an extern_()
helper like the one on Rustc
.
☔ The latest upstream changes (presumably #124577) made this pull request unmergeable. Please resolve the merge conflicts. |
I'd consider this PR blocked on #125798. |
Fixes #75341.
I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...).
The following tests are not included into the combined doctests:
compile_fail
#![no_std]
test_harness
--show-output
(because the output is nicer without the extra code surrounding it)Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately.
Because of the
edition
codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic.In case the users want a specific doctest to be opted-out from this doctest merge, I added the
standalone
codeblock attribute:Now the interesting part, I ran it on a few crates and here are the results (with
cargo test --doc
to only include doctests):r? @notriddle