Replaced version (unittest) with version (DRuntimeUnittest) to avoid unnecessary overhead when compiling with -unittest #2902

alexandrumc · 2020-01-08T16:49:04Z

Related to #2888 and #2896

dlang-bot · 2020-01-08T16:49:08Z

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 verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your 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 locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2902"

Geod24 · 2020-01-09T03:50:02Z

I appreciate the focus on this, however maybe we should pause and consider how widespread this problem is likely to be. Any library is likely to suffer from this. I'm thinking Vibe.d, for example.

Should the compiler blindly run unittests in all dependencies ? Is Phobos that special that we want this rule to apply to all dependencies but druntime/Phobos ? I don't think so. Running your dependencies' unittests is not scalable and rarely what you want, especially during development.

Can we start to acknowledge this and move to a solution on the language level, rather than pushing the problem down to all libraries ?

alexandrumc · 2020-01-09T06:31:05Z

Should the compiler blindly run unittests in all dependencies ? Is Phobos that special that we want this rule to apply to all dependencies but druntime/Phobos ? I don't think so. Running your dependencies' unittests is not scalable and rarely what you want, especially during development.

The compiler doesn't run unittests in all dependencies. For example, in the following code

module foo;

import std.smth;

unittest 
{
     import std.stdio : writeln;
     writeln("Target unittest: ", a);
}

version (unittest)
{
     int a  = 10;
}

void main() {}

module std.smth;

unittest
{
    import std.stdio : writeln;
    writeln("Dependency unittest: ", b);
}

version (unittest)
{
    int b = 5;
}

dmd -unittest foo.d; ./foo will only print Target unittest: 10. However, int b = 5 will be visible and semantic analyzed without any usefulness. This is why we want to remove / replace all the version (unittest) s from Phobos and druntime.

Geod24 · 2020-01-09T07:49:41Z

@alexandrumc : Right, it doesn't do it for non-root modules.
But that depends on your build system. If you're using dub with library you're fine, if you use rdmd (or dub with sourceLibrary, I'd wager) those are going to be root modules, and your unittests are going to be run.
Druntime is a bit of a special case here, as it's always linked, and its modules are never root modules.

alexandrumc · 2020-01-09T08:37:28Z

@alexandrumc : Right, it doesn't do it for non-root modules.
But that depends on your build system. If you're using dub with library you're fine, if you use rdmd (or dub with sourceLibrary, I'd wager) those are going to be root modules, and your unittests are going to be run.
Druntime is a bit of a special case here, as it's always linked, and its modules are never root modules.

Sure, that's true. But by changing the version (unittest)s I am not trying to address the issue you described. version {} and unittest{} are two different mechanisms and by chance two separate aspects of the same problem: the behaviour triggered by the -unittest switch. Here I am just trying to solve the problem that I described in my previous comment.

atilaneves · 2020-01-09T09:54:04Z

I appreciate the focus on this, however maybe we should pause and consider how widespread this problem is likely to be. Any library is likely to suffer from this. I'm thinking Vibe.d, for example.

Correct. This is why none of my own libraries have unittest blocks anywhere that client code could see.

Should the compiler blindly run unittests in all dependencies ?

No.

Is Phobos that special that we want this rule to apply to all dependencies but druntime/Phobos ?

Yes. It's the standard library, it's the dependency.

I don't think so. Running your dependencies' unittests is not scalable and rarely what you want,
especially during development.

The issue is semantic analysis, not actually running tests.

Can we start to acknowledge this and move to a solution on the language level, rather than pushing the problem down to all libraries ?

Yes. But for now this is easier. I need to discuss this with @WalterBright

Geod24 · 2020-01-09T10:07:43Z

src/core/time.d


 //To verify that an lvalue isn't required.
-version (unittest) private T copy(T)(T t)
+version (DRuntimeUnittest) private T copy(T)(T t)


I believe you need to keep this because it is called from a templated unittest.

At least that's what the CI suggests:

/var/lib/buildkite-agent/builds/ci-agent-89c4044a-12c8-4cab-b953-c7a634bd4891-5/dlang/druntime/build/distribution/bin/../imports/core/time.d(2202,21): Error: undefined identifier `copy`, did you mean import `core`? -- | /var/lib/buildkite-agent/builds/ci-agent-89c4044a-12c8-4cab-b953-c7a634bd4891-5/dlang/druntime/build/distribution/bin/../imports/core/time.d(2210,16): Error: undefined identifier `copy`, did you mean import `core`? | /var/lib/buildkite-agent/builds/ci-agent-89c4044a-12c8-4cab-b953-c7a634bd4891-5/dlang/druntime/build/distribution/bin/../imports/core/time.d(2211,16): Error: undefined identifier `copy`, did you mean import `core`? | /var/lib/buildkite-agent/builds/ci-agent-89c4044a-12c8-4cab-b953-c7a634bd4891-5/dlang/druntime/build/distribution/bin/../imports/core/time.d(2212,25): Error: undefined identifier `copy`, did you mean import `core`? | /var/lib/buildkite-agent/builds/ci-agent-89c4044a-12c8-4cab-b953-c7a634bd4891-5/dlang/druntime/build/distribution/bin/../imports/core/time.d(2272,20): Error: undefined identifier `copy`, did you mean import `core`? | /var/lib/buildkite-agent/builds/ci-agent-89c4044a-12c8-4cab-b953-c7a634bd4891-5/dlang/druntime/build/distribution/bin/../imports/core/time.d(2283,13): Error: undefined identifier `assertApprox` | /var/lib/buildkite-agent/builds/ci-agent-89c4044a-12c8-4cab-b953-c7a634bd4891-5/dlang/druntime/build/distribution/bin/../imports/core/time.d(2004,18): Error: template instance `core.time.MonoTimeImpl!cast(ClockType)0` error instantiating

I don't think that's actually a problem. I think that the -version=DRuntimeUnittest is somehow not passed to the compiler when unittests are compiled.

Are the Makefiles used for testing exactly the ones that are located in phobos/? Are they different?

For the record, I both of you are correct.
The root cause was that even though the function was used only from druntime's unit tests, they were nested inside the MonoTime template, which was instantiated from phobos, without passing -version=CoreUnittest. I solved this by guarding all nested (and undocumented) unit tests with version (CoreUnittest).

Geod24 · 2020-01-09T10:10:34Z

Yes. But for now this is easier. I need to discuss this with @WalterBright

@atilaneves : Great, thanks. And happy to know you guys are working on improving compilation speed!

wilzbach

CoreUnittest already exists! Besides only Core and Std are reserved version keyword prefixes.

PetarKirov · 2020-01-21T08:22:10Z

I addressed the issues with core.time.copy (see 795cc3a), rebased and squashed.

@alexandrumc let's wait and see if the CI will be green, hopefully no further work on this PR will be necessary from you.

It prevented removing all instances of `version (unittest)` from druntime as various nested unittest blocks were using `copy`, which under `version (CoreUnittest)` would not be visible from MonoTime instantiators, such as phobos. (See the next commit for additional details.)

To avoid unnecessary overhead when compiling with -unittest

Addressed.

PetarKirov · 2020-01-21T08:39:27Z

@alexandrumc @atilaneves @Geod24 @thewilsonator @wilzbach Please take another look.

PetarKirov · 2020-01-21T08:43:03Z

BTW, I just rembered about the discussion in this PR: dlang/phobos#6159. @atilaneves what's your take on it?

Edit: after re-reading that discussion, I still think that using CoreUnittest/StdUnittest is the right way to go for now, until we have a proper compiler-level solution, as that may take a long time to materialize.

…ion (CoreUnittest)`

PetarKirov

@alexandrumc's changes look good to me.

alexandrumc requested review from Burgos, CyberShadow, MartinNowak, WalterBright, andralex, ibuclaw, jmdavis, rainers, schveiguy and wilzbach as code owners January 8, 2020 16:49

Geod24 reviewed Jan 9, 2020

View reviewed changes

wilzbach previously requested changes Jan 12, 2020

View reviewed changes

alexandrumc mentioned this pull request Jan 19, 2020

Replaced version (unittest) with version (StdUnittest) to avoid unnecessary overhead when compiling with -unittest dlang/phobos#7353

Merged

PetarKirov force-pushed the replace-version-unittest branch from 8fc3345 to 8a0e873 Compare January 21, 2020 08:19

PetarKirov and others added 2 commits January 21, 2020 10:32

Replace version (unittest) with version (CoreUnittest)

df390c1

To avoid unnecessary overhead when compiling with -unittest

PetarKirov force-pushed the replace-version-unittest branch from 8a0e873 to df390c1 Compare January 21, 2020 08:33

[core.time]: Place all nested and undocumented unit tests under `vers…

ddf4bcc

…ion (CoreUnittest)`

PetarKirov approved these changes Jan 21, 2020

View reviewed changes

atilaneves approved these changes Jan 21, 2020

View reviewed changes

atilaneves added the auto-merge label Jan 21, 2020

dlang-bot merged commit 2fa6943 into dlang:master Jan 21, 2020

Uh oh!

Replaced version (unittest) with version (DRuntimeUnittest) to avoid unnecessary overhead when compiling with -unittest #2902

Replaced version (unittest) with version (DRuntimeUnittest) to avoid unnecessary overhead when compiling with -unittest #2902

Uh oh!

Conversation

alexandrumc commented Jan 8, 2020

Uh oh!

dlang-bot commented Jan 8, 2020

Bugzilla references

Testing this PR locally

Uh oh!

Geod24 commented Jan 9, 2020

Uh oh!

alexandrumc commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Geod24 commented Jan 9, 2020

Uh oh!

alexandrumc commented Jan 9, 2020

Uh oh!

atilaneves commented Jan 9, 2020

Uh oh!

Geod24 Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

alexandrumc Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

alexandrumc Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

PetarKirov Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Jan 9, 2020

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetarKirov commented Jan 21, 2020

Uh oh!

PetarKirov commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alexandrumc commented Jan 9, 2020 •

edited

Loading

PetarKirov commented Jan 21, 2020 •

edited

Loading

PetarKirov commented Jan 21, 2020 •

edited

Loading