[marler8997]

A backwards compatible alternative to #271

[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.

[marler8997]

Note: implementation is currently very "rough", still working on it.

[CyberShadow]

Thanks for your continued work on this matter.

Have you evaluated using -deps=filename? That would solve the redirect (linker/pragma) issues.

[marler8997]

It's seems to be missing to much information.

[CyberShadow]

Could improving -deps output still be the best way forward?

[marler8997]

Could be, but supporting verbose going to a file would be much less work:)

[CyberShadow]

Could be, but supporting verbose going to a file would be much less work:)

Yeah, but I can see the point in objections for that.

Currently, -v is a bit of an umbrella flag that enables a various bunch of any kind of information that "might" be useful to programs and users. That it does not have its own dedicated stream is just more proof about its ad-hoc-ness. Meanwhile, -deps seems to be semantically tied to the purpose we'd be using it for, and I don't think there can be any reasonable objections against improving and using it.

We probably should have made rdmd use -deps=file a long time ago, and all the improvements in -v output should have went in -deps. In fact, probably the reason that all -v improvements happened in -v and not -deps was only because of this specific rdmd misdesign.

Would it really be more complicated printing a line to -deps=file whenever we also print a relevant line to verbose output?

[marler8997]

You make some good points. I'll look into this more tomorrow. I suppose it wouldn't really be more complex now that you mention it.

[CyberShadow]

Thanks!

[marler8997]

I just remembered that the -deps flag also modifies compilation by fully analyzing all the imported modules whether or not they are needed for compilation. This would result in a performance hit.

[CyberShadow]

Err, but, we're already doing that because we're doing one-pass compilation anyway, right?

Also, is -deps=file really automatically enabling -deps?

[marler8997]

Were only adding semantic analysis on the modules that are being compiled. The biggest impact this would have is on druntime/phobos modules, forcing you to fully analyze all those modules who have a dependency even if they aren't being used.

Yes -deps=file would enable this just like -deps would.

[marler8997]

running a simple comparison, -deps takes 5.1 seconds to compile one of my projects, where as -v takes 2.1 seconds.

[marler8997]

Correction, most of the slow down was from printing all the information to the console. Apparently deps has ALOT more to print than -v. If I redirect everything to a file I get 1.9 seconds for -v and 3.5 seconds for -deps. Same time if I use -deps=file.

[marler8997]

I've "double checked" my suspicion that the extra semantic analysis is causing the slow down. There's 2 lines in mars.d you can comment out to disable the extra semantic analysis, when I commented those out, the performance of -deps and -v is identical.

[CyberShadow]

Hmm, well, that just tells us that we've made a mistake by bundling that behaviour with the -deps switches.

I recall that this (enabling deep scanning with -deps) was done fairly recently. Do you think it's not too late to move that to a separate flag?

if not, then we can make the flag a toggle, like -boundscheck / -boundscheck=off.

[marler8997]

Good question, might try to ask around. Also Walter says we could use the JSON file for this, will look into that as well.

[wilzbach]

What happened to just using -i by default and falling back to the old mechanism if that fails with "unknown flag"? It comes without a performance penalty for the default and main path.

[CyberShadow]

That doesn't solve the linker output problem.

[marler8997]

We could go with that too. I left a comment in the code if we go that route. The only problem with that one is now we have to redirect stderr, but might not matter. But the extra compiler invocation to get the version may be negligible, likely affecting final time less than a millisecond since it's caching the file access anyway

[marler8997]

What doesn't solve the linker problem?

[wilzbach]

If the linker output problem gets addressed before 2.079 gets fixed, you wouldn't have to worry about that...

[CyberShadow]

What happened to just using -i by default and falling back to the old mechanism if that fails with "unknown flag"? It comes without a performance penalty for the default and main path.

Essentially this is unrelated and orthogonal to the question of "How do we get the list of dependencies, now that we invoke the compiler exactly once?". Parsing stdout/stderr is complicated because we have to be mindful of letting compiler errors, linker output, and user pragma(msg) through while parsing and filtering out -v output meant for rdmd to get the list of dependencies.

[CyberShadow]

If the linker output problem gets addressed before 2.079 gets fixed, you wouldn't have to worry about that...

I don't understand what you mean by that. How would the linker output problem "get addressed", and how would 2.079 "get fixed"?

[marler8997]

Right

[wilzbach]

I thought Jonathan was working on making the linker write to stderr or do use sth like a "L:" prefix for all linker output, no?

[marler8997]

There's alot going on, it's hard to keep track. I am exploring both of these issues.

[marler8997]

@andralex @wilzbach @timotheecour

Wanted to get your guys' opinion on this. So far we've had 2 options proposed to allow rdmd to work with multiple versions of the 3 D compilers. 1) Have rdmd query the compiler version before invoking it, or 2) use the latest interface and fallback to the legacy interface when a failure is detected.

This PR currently uses option 2, however, as I've been looking through the code I've seen bug fixes and other feature enhancements that could be using other features that were only made available in certain versions of the compiler (the -deps flag for instance). Given that any enhancements to rdmd need to maintain a certain amount of backwards compatibility with older versions of the compiler, I'm thinking that we probably should have gone with option 1. This way we can turn on/off independent features based on the version.

I do want to note that the performance impact of checking the compiler version will likely be negligible. Andrei pointed out that even though we have to deal with the overhead of creating an extra process, it will make the next invocation faster since it has "warmed up" the file-system/memory to load that process again. And if process creation has a noticeable impact we can cache the result of the version check.

I also want to mention that this change does complicate rdmd a fair amount. Every time we support a new feature specific to a version of the compiler, we are doubling our test surface. This work is turning out to be a delicate balancing act.

[andralex]

Detecting the version scales forward nicely; future features can use that as well.

[marler8997]

Ok, so long as there are no objections, we'll go with detecting the version.

[marler8997]

FYI, I've added the compiler version to the json file so we could use that moving forward. If the json file was cached, get the version from that, otherwise invoke the compiler to get the version.

[timotheecour]

here's a more robust way to solve these use case (figuringing out if compiler has feature X like allowing -i or -deps or -deps=file, etc):

// pseudocode
execute(`dmd -Xf=query.json`);
bool has_dash_i = `query.json`.readText.parseJson["features"].canFind("-i");

Advantage: more robust:

• if we later deprecate -i (or some other feature) in favor of some other one (or change name etc), checking the compiler version will not work (without more logic on rdmd side), this will
• if another compiler (eg gdc) wants to implement -i before getting the frontend version up to date with dmd, checking the compiler version will not work, this will
• no need to distinguish between whether we use compiler version or frontend version
• also works seamlessly if a feature is only available in a certain environment (eg linux), whereas checking compiler version would not work (and move complexity to client code (eg rdmd) to check if (version>2.077 and OS is linux) ...
• would allow feature detection to be dependent on -version flags; eg:
  if user calls rdmd -version=allow_experimental_feature_X main.d , rdmd will call execute("dmd -Xf=query.json "~ version_arguments_from_cmd_line); and detect with compiler has feature X with these version flags

NOTE: this requires dmd -Xf=query.json to work without having to pass a file (eg main.d) to dmd. I think it makes sense to allow that anyways. And also requires adding some additional entries to JSON output, which is fine.
NOTE: this other PR dlang/dmd#7521 (comment) is introducing a new flag -probe which is trying to solve basically the same problem. Both use cases could be solved via -Xf=file or via -probe.

[wilzbach]

The disadvantages of -Xf is that generating and writing the json file isn't for free - especially if you don't need to look at it.

NOTE: this other PR dlang/dmd#7521 (comment) is introducing a new flag -probe which is trying to solve basically the same problem. Both use cases could be solved via -Xf=file or via -probe.

Yeah, but -probe (or -version=json) would be cheaper to call (and doesn't create temporary files) ;-)

[wilzbach]

dmd -Xf=query.json`

Error: no object files to link

[timotheecour]

we would of course allow -Xf with no other arguments in support of this.
Regarding speed, I dont see an issue at all because we'd cache the json output, keyed by a hash of compiler path and time of last modification.

[wilzbach]

we would of course allow -Xf with no other arguments in support of this.

Which is then essentially -probe or -version=json - just with a different name ;-)

[CyberShadow]

As far as I understand, we don't need -probe. Just have -Xf- and no other arguments do the same thing as -probe, i.e. write some general JSON about the compiler to stdout.

[wilzbach]

Yes, but -Xf- should return at least the things I planned to return for -probe - otherwise dub wouldn't be able to use it.

[CyberShadow]

Then perhaps we should do that, then. Anything useful for Dub and planned for -probe is likely to be useful enough to include in all JSON output.

[timotheecour]

exactly.
dlang/dmd#7757 (Add more stuff to JSON file) was just submitted; you can add the remaining fields that are needed for dub to the JSON output, as done in that PR, and optionally support -Xf=- for outputting to stdout and also allow outputting json when no input files/objects are given

[marler8997]

Yes, but -Xf- should return at least the things I planned to return for -probe - otherwise dub wouldn't be able to use it.

Definitely. Let's add it to the JSON file soon before the next release so you don't have to support different versions of the JSON file.

[marler8997]

Based on the resistance I get when pushing changes to this repo, I've decided my time is better spent elsewhere.