Add -unittest=rootonly and -unittest=first flags#10937
Add -unittest=rootonly and -unittest=first flags#10937alexandrumc wants to merge 8 commits intodlang:masterfrom
Conversation
alexandrumc
requested review from
Geod24,
MartinNowak,
WalterBright,
ibuclaw and
rainers
as code owners
|
Thanks for your pull request and interest in making D better, @alexandrumc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#10937" |
Do we really need to solve this problem? Adding more and more flags and conditions just makes things confusing. Unittest should be as simple as practical. |
|
Agreed with @WalterBright . I don't think exposing the complexities of template instantiation is a step forward. |
|
I think we need to fix this problem because one of D's strategic advantages are fast compile times, but they suffer immediately when |
|
The issue with unittests is simpler IMO. We simply should not compile unittests in imported modules period. This is exactly how it works for non templated unittests. This is where the slowdown comes from. See previous work here: #8124 |
That's not what @alexandrumc found. |
What did he find? I found that non-templated unittests inside imports were not semantically analyzed. I had thought they were, but discovered it only happens for templates. |
|
@schveiguy rereading the comment and taking it at face value. I understood it to be the following scenario. Module A instantiates template FN in normal code, but compilation does not generate code for it because imported module B also instantiates the template with the same constraints. Though I disagree that the above I described would increase binary size, as duplicates are discarded at link time. There's a bug report somewhere that explains the historical why it is necessary to always generate code if unittests are being compiled too. However off the top of my head, I'd say it makes more sense to do this when |
Yes, that's exactly why it takes more time to compile.
Yeah, that's probably correct. It's my mistake. Now I realize that it's not the binary size which is increased but the obj/lib files. (e.g. when Phobos is compiled for unit testing, it is compiled one module at a time using
I couldn't think of another reason than to avoid link-time errors. |
|
The problem that I've seen is that instantiated templates included their unittests even if they were imports. And it was really the semantic analysis that caused the slowdown. I'm trying to verify what actually happens with when I remember happening, but my various compiler installations are not all working. There were cases where unittests were being semantically analyzed of imported templates, which in turn imported more stuff and then blew up into a huge problem. But I can't remember or re-prove what was happening. At this point, it appears that templates that are not instantiated locally do not include instantiated unittest semantic analysis, which is good. In the past it was included, but I don't know when that was stopped. Imported templates that are instantiated locally still run unittests of those templates. I still think that should be stopped, there's no reason to test imported types that have documenting unittests just because you instantiated them locally. |
|
OK, so the analysis is still done in certain cases. So for example: module mod1;
struct S()
{
unittest { pragma(msg, "analyzed"); }
}
version(test1) alias sinst = S!();
version(test2) void foo(S!() s) {}And the importing module: import mod1;
version(test3) alias sinst = S!();
void main() {}So let's compile mod1 like this: No instantiations, no unittests. What happens when you compile main.d?
So IMO, the most damaging one is test2. You are semantically analyzing the unittest, which means importing and compiling all it requires without actually including the unittest. test3 is also not ideal, I don't think there's any reason to run unittests of imported modules period. |
Thanks for the detailed explanation. I've compiled (the same way you did) using the changes in this PR (with |
Yes, but I'm saying that should be the way it ALWAYS is, not just behind a switch. |
|
I suppose I should clarify a bit. Yes, I agree that only root unittests should be compiled. In response to Walter, that is the simplest possible solution. I can't seen any use case to compile unit tests in imported modules that aren't being included as part of the compilation unit. I understand that this PR does this for a specialized switch -- I think it should be always. I don't think the And I don't buy that the emitting of the compiled code to object files is the problem, I think it's the semantic analysis. This PR solves both, but I think a simpler solution could solve the most expensive one. |
BTW, version(unittest) should NOT be turned off if -unittest or -unittest=anything is on. It's too confusing/disruptive to do this. |
I guess you solve the semantic analysis you solve both :) So yeah, my complaint really just falls back on, why don't we always do what |
|
Let's see what the testsuite says first before making it the default. |
|
Only if those templates are instantiated, no ? |
Yes, but that's not so uncommon to see. This is why I changed all the |
I've changed the This is why I think this flag shouldn't directly replace |
|
Those failures seems to indicate another problem. Just looking at two random failures: This test does not use templates:
Likewise, this test doesn't involve templates:
|
|
Yes, and that's on the library developer to design properly. Note that a library author could include unittesting facilities for their library in case you want to integration test your project with their library. Or it might be something that's contained into one module for use in testing several modules, and only imported inside unittests. There are definitely ways to use version(unittest) that don't impact importing modules, and there are ways to do it badly. But we should not restrict the feature, especially when there are valid uses. |
|
This seems to be outdated, given the recent template emission fixes. |
8 participants
The default instantiation strategy (no
-unittest) is to prefer the template instances that are instantiated in non-root modules in order to minimize the binary size and the compilation time (there are fewer symbols which require semantic analysis).However, when the
-unittestflag is enabled, the strategy is to prefer the template instances that are instantiated in root modules (this leads to larger binaries and compilation times but guarantees no link-time failures).To avoid the compilation time and the binary size increase I propose two new flavors of the
-unittestflag which use the default instantiation strategy and provide more flexibility in testing.-unittest=rootonlyenables only theversion(unittest) {}and theunittest {}blocks that reside inside the root modules.-unittest=firstenables only theversion(unittest) {}and theunittest {}blocks that reside in the first root module provided in the command line. This is useful if one wants to test only a single module but also has to provide other root modules for linking reasons.