Use "-i" to prevent rdmd from having to invoke compiler twice.

[marler8997]

Should result in a big performance improvement to rdmd (about 30% decrease in run time). Depends on this commit from dmd (dlang/dmd#7099).

[dlang-bot]

Thanks for your pull request, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

[CyberShadow]

I tried to run the rdmd unit tests of this + dlang/dmd#7099:

digger build master+dmd#7099+tools#271
digger test --without=dmd --without=druntime --without=phobos

The rdmd test suite fails:

core.exception.AssertError@rdmd_test.d(179)
----------------
??:? _d_assert_msg [0x4aba26]
??:? void rdmd_test.runTests() [0x488cba]
??:? _Dmain [0x48745f]
object.Exception@/home/vladimir/work/extern/D/import/ae/sys/d/manager.d(844): Command ["dmd", "-run", "rdmd_test.d"] failed with status 1
----------------

It looks like an issue with --exclude:

tools/rdmd_test.d

Lines 178 to 179 in f1d60cd

	res = execute(rdmdArgs ~ ["--force", "--exclude=dsubpack", subModUser]);
	assert(res.status == 1, res.output); // building without the dependency fails

[marler8997]

Yeah I already kinds knew it was going to break exclude. Was going to wait until the dmd PR was integrated until I finish implementing everything in this PR :)

[CyberShadow]

Are you sure that the dmd PR has everything needed, though, and we won't run into some issue after it's merged?

Having the rdmd test suite pass would give us some guarantee of completeness, and is probably the best metric we have for measuring that.

[marler8997]

Yeah that's a good point. At the moment I'm trying to add some tests in the dmd repo for the new feature. I'll see if I can finish the rdmd implementation after that.

[marler8997]

Got the tests to pass now. I also took some performance measurements before and after this change with the following command:

time rdmd -g -debug rdmd_test.d --compiler ~/ddev1/dmd/generated/linux/release/64/dmd

Before Change

real    0m13.033s
user    0m11.720s

real    0m13.165s
user    0m11.848s

After Change

real    0m9.411s
user    0m8.392s

real    0m9.236s
user    0m8.244s

About a 30% decrease in run time :)

[wilzbach]

Looks really good!

Pro tip: use this url to look at the diff - https://github.com/dlang/tools/pull/271/files?w=1

[wilzbach]

How about letting tryGetDependencies and readDepsFile return a const(string[string] instead of the "ugly" cast?

[marler8997]

Wouldn't that just move the cast to the tryGetDependencies and readDepsFile functions?

[wilzbach]

It works for me:

diff --git a/rdmd.d b/rdmd.d
index 3bc7460..3e17fbd 100755
--- a/rdmd.d
+++ b/rdmd.d
@@ -313,7 +313,8 @@ int main(string[] args)
 
     if(makeDepend || makeDepFile !is null)
     {
-        auto resolvedDeps = (initialDeps is null) ? cast(const(string[string]))readDepsFile(objDir, depsFilename) : initialDeps;
+        auto resolvedDeps = (initialDeps is null) ? readDepsFile(objDir, depsFilename) : initialDeps;
         assert(resolvedDeps !is null, "code bug: dependencies were not properly generated from the build");
 
         // --makedepend mode. Just print dependencies and exit.
@@ -610,7 +611,7 @@ private int exec(string[] args)
     return run(args, null, true);
 }
 
-string[string] readDepsFile(string objDir, string depsFilename)
+const(string[string]) readDepsFile(string objDir, string depsFilename)
 {
     string d2obj(string dfile)
     {
@@ -705,7 +706,7 @@ string[string] readDepsFile(string objDir, string depsFilename)
 // directory workDir. The mapping is obtained by reading the 'rdmd.deps' file
 // that would have been generated on a previous build.
 
-private string[string] tryGetDependencies(string rootModule,
+private const(string[string]) tryGetDependencies(string rootModule,
         string objDir, string[] compilerFlags, string depsFilename)
 {
     // Check if the old dependency file is fine

[wilzbach]

I assume this was just for development / testing?

[marler8997]

Actually it's necessary. Before this change the compiler was called using -v for the "first call" to the compiler, now both calls are combined into a single call. rdmd still uses the output from -v to read the dependencies to know when a rebuild is necessary.

[wilzbach]

Ah fair enough. Thanks for the explanation!

[wilzbach]

I also took some performance measurements before and after this change with the following command:

Looks amazing!

Before we merge this, we should add a changelog entry mentioning the awesome speed up. Maybe you are even interested in preparing a short article for the next release on the blog?
CC @mdparker
(@mike: the main story is at dlang/dmd#7099)

[marler8997]

An article? Hmm, maybe...

[mdparker]

@marler8997

An article? Hmm, maybe...

Yes, please!

[timotheecour]

@marler8997 will rdmd's --exclude --include options become redundant after this is merged, since we could use -i=-core,core.sys ? (with added benefit of fixing https://issues.dlang.org/show_bug.cgi?id=18040 )
if so we should probably start deprecating path for these options.
will it fix https://issues.dlang.org/show_bug.cgi?id=18042?

[marler8997]

Yes, --exclude --include will become redundant and we should deprecate them. You may or may not have noticed, but this change simply forwards those arguments to -i.

I haven't tested it yet but I think it will also fix https://issues.dlang.org/show_bug.cgi?id=18042

[marler8997]

@MartinNowak I'm trying to triage the jenkins failure, but after I logged in I get this mesesage:

Access Denied
marler8997 is missing the Overall/Read permission

[wilzbach]

Yeah you need write access to restart jobs, I just did this for you. You can always force-push the same commit to kick the CIs

[ibuclaw]

What regression are you talking about that the PR would fix?

That patch was spun from problems with this patch. To quote yourself:

However, it appears that OPTLINK prints linker errors to stdout, which means those errors get redirected to the dependency file.

Ergo, regression. While its obvious that optlink is the culprit that needs fixing, there still needs to be consideration in supporting older versions of tools going back at least two years (I think 2 years is fairly relaxed a constraint, but project maintainers of rdmd may be more lax or strict as they see fit).

I see no direct approach to take, every move should be treated like chess pieces. You make a change with the intent to make use of in the future. You've pushed your change in the compiler, so the natural thing to do is wait until that change becomes ubiquitous.

[marler8997]

There still needs to be consideration in supporting older versions of tools going back at least two years (I think 2 years is fairly relaxed a constraint, but project maintainers of rdmd may be more lax or strict as they see fit).

Yes, that PR is not a full solution to the problem. I figured that I should get the support in now and maybe in 2 years we can start using it :) Until then we will need to settle for a backwards compatible approach such as filtering messages, or we could rely on fixing OPTLINK and say that it's ok to drop linker errors if you are using an old version of OPTLINK. We have options. That PR gives us more options for this one, but like I said, it should ultimately be considered independent of this one.

[timotheecour]

(andralex) the redirection to file would be useful if several unrelated things were output to stdout, is that the case?

yes: dlang/dmd#7746 (comment)

[WebDrake]

Some improvements to the test suite that should make it easier to run rdmd tests using multiple D compilers: #293

[marler8997]

Yes please :)

[timotheecour]

this PR is obsoleted by #292 but comments here still hold

[timotheecour]

DYLD_LIBRARY_PATH on OSX

would be nice to abstract out OS specific things eg:

foreach (envVar; ["LIB", "LIBRARY_PATH", os_LD_LIBRARY_PATH]) {...}

module rdmd.util;
string os_LD_LIBRARY_PATH(){ // find a better name
version(OSX) return "DYLD_LIBRARY_PATH";
else version(linux) return "LD_LIBRARY_PATH";
else assert(0);
}

[timotheecour]

.dylib on OSX
same comment as above:

string[] names = ["lib" ~ libName ~ ".a", "lib" ~ libName ~ os_so_ext];

[CyberShadow]

@ibuclaw Does gdmd support -i now? Can we add this into rdmd today?

[Geod24]

To me the question becomes: Why do we need RDMD now ?

[CyberShadow]

It's a tool that supports a different flow that some people (incl. me, and Andrei I guess) are used to. Without rdmd, there's nothing in-between "use the compiler directly" (for single-file programs, no caching) and "use this build tool / package manager which requires creating a project in its own directory with dub.sdl and a certain directory structure etc.".

I don't use rdmd but I use @marler8997's rdmd-like build tool rund (now part of @dragon-lang apparently) which is essentially rdmd with -i support.

[CyberShadow]

Oh, just realized I misunderstood your question (thought you asked, why do we need rdmd today).

The main gimmick is that rdmd does caching, so you can put it in the shebang of D programs and that will only be slow on the first run. There's also a bunch of minor features.

[PetarKirov]

If dmd started doing caching (e.g. like go, nim and others), what benefits would rdmd offer on top?

Btw, I would have preferred if -i is not part of the compiler, but instead of a specialized build tool that uses the compiler as a library, similar to how bundlers work in JavaScript ecosystem. In JavaScript (or e.g. Dart) developers are used to HMR (starting the build tool in watch mode and having it do hot module replacement - i.e. rebuilding only the files that actually changed and replacing them in the running program).

[CyberShadow]

Not much, I think the main issue is that rdmd is a bit opinionated in how it does caching.

And, I agree. Either a library or some kind of subprocess protocol like LSP.

[ssvb]

@Geod24 other than having no caching feature, dmd -run attempts to write temporary files to the current directory and this may be problematic: https://forum.dlang.org/post/[email protected]

[ssvb]

And another reason to avoid dmd -run in general: dlang/dmd@f3a0366