rdmd: compile the root module in the same step as getting the dependencies #194

aG0aep6G · 2016-08-27T23:31:35Z

As suggested by Andrei in a comment on #191. Consequently, this is an alternative to #191.

Little benchmark

Used bash's builtin time with TIMEFORMAT=%R to get simple output.
All times are the best of 3 runs.
rdmd_master is the current master (1260719).
rdmd_single_out_root is the one from this PR.
foo.d is http://sprunge.us/ZZhR, taken from #191.

single file

$ time ./rdmd_master --force foo.d > /dev/null
1.142
$ time ./rdmd_single_out_root --force foo.d > /dev/null
0.747

Good improvement here.

trivial root, complex import

bar.d is import foo;.

$ time ./rdmd_master --force bar.d > /dev/null
0.935
$ time ./rdmd_single_out_root --force bar.d > /dev/null
0.937

As expected, no change here.

complex root, trivial import

baz.d is foo.d plus import qux;.
qux.d is an empty file.

$ time ./rdmd_master --force baz.d > /dev/null
1.144
$ time ./rdmd_single_out_root --force baz.d > /dev/null
0.763

This is improving too, because the root isn't compiled again when the import is compiled.

reuse cached executable

$ time ./rdmd_master foo.d > /dev/null
0.013
$ time ./rdmd_single_out_root foo.d > /dev/null
0.012

No bad surprise here.

This avoids calling dmd twice when running a single file, which is a common task.

dlang-bot · 2016-08-27T23:31:53Z

@aG0aep6G, thanks for your PR! By analyzing the annotation information on this pull request, we identified @CyberShadow, @andralex and @NilsBossung to be potential reviewers. @CyberShadow: The PR was automatically assigned to you, please reassign it if you were identified mistakenly.

(The DLang Bot is under development. If you experience any issues, please open an issue at its repo.)

wilzbach · 2016-08-27T23:35:43Z

As suggested by Andrei in a comment on #191. Consequently, this is an alternative to #191.

Oh nice - I did the same too, but didn't publish it yet because I wanted to test it further & do more cleanup ;/

AndrejMitrovic · 2016-08-27T23:45:51Z

rdmd.d

+    string[] objs = [ rootObj ];
+
+    // compile dependencies
+    if (myDeps.byValue.any) // if there is any source dependency at all


Is there a difference between using .byValue.any and .length > 0 ?

myDeps is a string[string]. .byValue.any is false when there are values but they're all null. Null values are set for non-source dependencies (lines 656-683).

But maybe this should check against emptiness instead. It's easy to forget that null has a different truthiness than empty strings.

That's quite a clever trick to check for "external" source files!

But maybe this should check against emptiness instead. It's easy to forget that null has a different truthiness than empty strings.

Yep better use .any!"a !is null", then it's at least consistent with the query below.

Yep better use .any!"a !is null", then it's at least consistent with the query below.

I think that's the same as just .any. But it's clearer with an explicit !is null, so I changed it. I did o => o !is null though, because I don't like string predicates.

AndrejMitrovic · 2016-08-27T23:52:56Z

Cool stuff! It gets my LGTM.

wilzbach · 2016-08-28T03:20:15Z

rdmd.d

+    auto depsGetter = [ compiler ] ~ compilerFlags ~ [
+        "-v",
+        "-c",
+        "-of"~buildPath(objDir, "rdmd.root"~objExt),


I used exeBasename as the filename is shown in the error message

I used exeBasename as the filename is shown in the error message

Seems like a good idea. Done.

The problem I had worked around here was that dmd would also write to foo.o when -main is passed in the linking command.

I've now changed it to pass -main when compiling the root module. But there seems to be a bug in dmd with -main -c -of: issue 16440. So I've reverted to rdmd's old style of doing --main: stubmain.d.

wilzbach · 2016-08-28T04:42:10Z

It's too late today for me to really look at rdmd.d anymore. Yours here seems to be more extensive than mine. So you may have caught details that I missed.

In the short version: You directly modified the source dependency checking function to emit an object file, whereas I went a rather long road that removes this function completely and refactored quite a bit on the way.

PR duel!

After comparing our approaches I think yours is better because it touches a lot less lines - great job!
If I find time, I will port my refactorings to the future master.

I already added a couple of comments, but there is one interesting thing in the benchmarks I did at #195 - your binary is 0.1MB larger and it takes 1ms longer to run on all benchmarks.
I store the information whether there are other source files directly while dependency file is read, might this explain the 1ms difference?

edit: just to clarify - the approach #195 is the same and thus I can backup the benchmarks and improved performance.

Moving --main functionality from linking to compilation of root module so that dmd doesn't mess with the object files.

aG0aep6G · 2016-08-28T11:31:49Z

I already added a couple of comments, but there is one interesting thing in the benchmarks I did at #195 - your binary is 0.1MB larger and it takes 1ms longer to run on all benchmarks.
I store the information whether there are other source files directly while dependency file is read, might this explain the 1ms difference?

Are you sure that you can detect a 1ms difference? On my machine, two runs of the same binary usually differ by more than 1ms.

I've ran your benchmark script a couple times, and the results weren't really conclusive. Sometimes #195 was faster, other times #194 was faster. The difference was always only a couple ms. Two runs of the same binary had a similar variance.

andralex · 2016-08-28T21:55:56Z

rdmd.d

@@ -470,46 +479,58 @@ private int rebuild(string root, string fullExe,
        }
    }

-    auto fullExeTemp = fullExe ~ ".tmp";
+    immutable fullExeTemp = fullExe ~ ".tmp";
+    immutable rootObj = buildPath(objDir, root.baseName(".d")~objExt);


spaces around ~ please

andralex · 2016-08-28T21:59:12Z

Good work, I approve modulo a few nits. Thanks!!

aG0aep6G · 2016-08-28T22:59:03Z

Good work, I approve modulo a few nits. Thanks!!

Added spaces around ~ (but only at the ones I'm introducing or touching anyway). That was the only nit, right?

andralex · 2016-08-28T23:40:16Z

thx!

andralex · 2016-08-28T23:41:04Z

This is definitely worth a changelog entry and hopefully also a blog entry.

wilzbach · 2016-08-29T00:05:04Z

This is definitely worth a changelog entry and hopefully also a blog entry.

Ping @mdparker - we had quite a fun weekend here. Maybe you can interview @aG0aep6G?

mdparker · 2016-08-30T12:08:58Z

If @aG0aep6G sends me an email (I can't find any contact info), we can start talking about a blog post. I'm eager to do so.

aG0aep6G · 2016-08-30T14:35:31Z

If @aG0aep6G sends me an email (I can't find any contact info), we can start talking about a blog post. I'm eager to do so.

My email is ag0aep6g@gmail.com. Though, I'm not sure if I can give much interesting insight. After all, combining -c with -v was Vladimir's/Andrei's idea.

CyberShadow · 2016-12-17T14:36:35Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=16978

This reverts commit a63233c, reversing changes made to 6f92f7f.

compile the root module in the same step as getting the dependencies

10ca158

This avoids calling dmd twice when running a single file, which is a common task.

dlang-bot assigned CyberShadow Aug 27, 2016

aG0aep6G mentioned this pull request Aug 27, 2016

Fix issue 16431 - Speed-up rdmd for single file builds #191

Closed

wilzbach mentioned this pull request Aug 27, 2016

[WIP] Fix issue 16431 - Get dependency output directly from the dmd run #195

Closed

AndrejMitrovic reviewed Aug 27, 2016
View reviewed changes

wilzbach reviewed Aug 28, 2016
View reviewed changes

aG0aep6G added 2 commits August 28, 2016 12:16

explicit !is null

4ab9444

name object files after root module

821d038

Moving --main functionality from linking to compilation of root module so that dmd doesn't mess with the object files.

andralex reviewed Aug 28, 2016
View reviewed changes

spaces around ~ please

0e44ee7

missed one

4da871d

andralex merged commit a63233c into dlang:master Aug 28, 2016

aG0aep6G deleted the rdmd-single-out-root branch August 31, 2016 12:44

CyberShadow added a commit to CyberShadow/tools that referenced this pull request Dec 17, 2016

Revert "Merge pull request dlang#194 from aG0aep6G/rdmd-single-out-root"

f24d9fa

This reverts commit a63233c, reversing changes made to 6f92f7f.

CyberShadow added a commit to CyberShadow/tools that referenced this pull request Dec 17, 2016

Revert "Merge pull request dlang#194 from aG0aep6G/rdmd-single-out-root"

8a5b9d6

This reverts commit a63233c, reversing changes made to 6f92f7f.

aG0aep6G mentioned this pull request Dec 17, 2016

rdmd fixes (issues 14296, 16962 and 16978) #207

Merged

mihails-strasuns mentioned this pull request Jan 16, 2017

Add support for configuring static/dynamic linking of libclang jacob-carlborg/dstep#105

Merged

wilzbach mentioned this pull request Feb 18, 2017

Bump dmd to 2.074.0 dlang-tour/core-rdmd#3

Merged

This was referenced Jun 3, 2017

Use dmd -run instead of rdmd dlang-tour/core#528

Merged

Use dmd -run instead of rdmd dlang-tour/core-rdmd#8

Merged

marler8997 mentioned this pull request Aug 25, 2017

Enable automatic compiling of imports dlang/dmd#7099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rdmd: compile the root module in the same step as getting the dependencies #194

rdmd: compile the root module in the same step as getting the dependencies #194

aG0aep6G commented Aug 27, 2016

dlang-bot commented Aug 27, 2016

wilzbach commented Aug 27, 2016

AndrejMitrovic Aug 27, 2016 •

edited

Loading

aG0aep6G Aug 28, 2016

wilzbach Aug 28, 2016

aG0aep6G Aug 28, 2016

AndrejMitrovic commented Aug 27, 2016

wilzbach Aug 28, 2016

aG0aep6G Aug 28, 2016

wilzbach commented Aug 28, 2016 •

edited

Loading

aG0aep6G commented Aug 28, 2016

andralex Aug 28, 2016

andralex commented Aug 28, 2016

aG0aep6G commented Aug 28, 2016

andralex commented Aug 28, 2016

andralex commented Aug 28, 2016

wilzbach commented Aug 29, 2016

mdparker commented Aug 30, 2016

aG0aep6G commented Aug 30, 2016

CyberShadow commented Dec 17, 2016

rdmd: compile the root module in the same step as getting the dependencies #194

rdmd: compile the root module in the same step as getting the dependencies #194

Conversation

aG0aep6G commented Aug 27, 2016

Little benchmark

single file

trivial root, complex import

complex root, trivial import

reuse cached executable

dlang-bot commented Aug 27, 2016

wilzbach commented Aug 27, 2016

AndrejMitrovic Aug 27, 2016 • edited Loading

Choose a reason for hiding this comment

aG0aep6G Aug 28, 2016

Choose a reason for hiding this comment

wilzbach Aug 28, 2016

Choose a reason for hiding this comment

aG0aep6G Aug 28, 2016

Choose a reason for hiding this comment

AndrejMitrovic commented Aug 27, 2016

wilzbach Aug 28, 2016

Choose a reason for hiding this comment

aG0aep6G Aug 28, 2016

Choose a reason for hiding this comment

wilzbach commented Aug 28, 2016 • edited Loading

aG0aep6G commented Aug 28, 2016

andralex Aug 28, 2016

Choose a reason for hiding this comment

andralex commented Aug 28, 2016

aG0aep6G commented Aug 28, 2016

andralex commented Aug 28, 2016

andralex commented Aug 28, 2016

wilzbach commented Aug 29, 2016

mdparker commented Aug 30, 2016

aG0aep6G commented Aug 30, 2016

CyberShadow commented Dec 17, 2016

AndrejMitrovic Aug 27, 2016 •

edited

Loading

wilzbach commented Aug 28, 2016 •

edited

Loading