Skip to content

remove -Z input-stats; replace with logging #37426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

zackmdavis
Copy link
Member

In Issue #34121, it was suggested that the input-stats debugging command-line flag could be replaced with logging.

Before:

$ rustc -Z input-stats scratch.rs
Lines of code:             5
Pre-expansion node count:  13
Post-expansion node count: 106

After:

$ RUST_LOG='rustc_driver=info' ./x86_64-unknown-linux-gnu/stage1/bin/rustc scratch.rs | grep ::
INFO:rustc_driver::driver: Lines of code:             5
INFO:rustc_driver::driver: Pre-expansion node count:  13
INFO:rustc_driver::driver: Post-expansion node count: 106
$ ./x86_64-unknown-linux-gnu/stage1/bin/rustc -Z input-stats scratch.rs
error: unknown debugging option: `input-stats`

You might wonder why the diff uses info! logging rather than debug! logging, as the issue suggested! It's because when I try to use debug!, it doesn't work: the messages don't get output even with RUST_LOG=debug or similar. I have no idea why this should be.

In Issue rust-lang#34121, it was suggested that the `input-stats` debugging
command-line flag could be replaced with logging.
@rust-highfive
Copy link
Contributor

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum
Copy link
Member

Note that http://perf.rust-lang.org/'s benchmarking for one currently passes this last I checked; while it's replaceable, it's something that'll need to be done once this is merged.

@nagisa
Copy link
Member

nagisa commented Oct 27, 2016

It's because when I try to use debug!, it doesn't work

debug! have no effect in builds which haven’t any debug info (its more compilcated than that, but this is a good enough summary), which is likely the case with your builds.

@alexcrichton
Copy link
Member

@Mark-Simulacrum to clarify, would this hard to update perf.r-l.o for? Or would it be a relatively minor change?

In general I personally at least like the idea of using logging for various stats like this rather than -Z flags.

r? @nrc, you may have thoughts as well!

@rust-highfive rust-highfive assigned nrc and unassigned Aatch Nov 10, 2016
@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 10, 2016
@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum to clarify, would this hard to update perf.r-l.o for? Or would it be a relatively minor change?

I don't believe these values are ever displayed in the output, so in some ways this change is "trivial" for perf.r-l.o, but it may break some parsing code (which I haven't touched, so can't comment on). @nrc may know more; I can look into it as well when I have a chance.

@nrc
Copy link
Member

nrc commented Nov 11, 2016

The intended use of the flag was for perf.r-l.o, the idea was to take this info and display along with the times/memory uses to give some context for the times taken. I guess we never implemented the code to actually parse and display this information on perf.r-l.o (iirc, I think we might parse, but not display).

This PR would make it much more annoying to do that - you'd potentially get a whole bunch of other info output. This mechanism is also rather easy to break without noticing. So, I think we shouldn't make this change. However, perhaps it is not worth having this info at all? In which case we could check the perf code, and then remove the option.

  • info! does seem more appropriate than debug! (this is information for the sake of information, not logging.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Nov 11, 2016

Not absolutely certain, but I believe that the scripts for parsing this for perf.r-l.o will not break if it's removed, except for the potential breakage when -Zinput-stats itself is removed.

I support not removing this because of the problems @nrc mentions above; neither info! nor debug! are restricted to just this output, so an additional problem there is that output size will grow, hence increasing the size of the github repository storing this data (faster than currently).

@steveklabnik
Copy link
Member

Ping! It's been almost two months. What should we do about this PR?

@zackmdavis
Copy link
Member Author

@nrc and @Mark-Simulacrum's comments are opposed, which would seem to outweigh @alexcrichton's neutral-to-mildly-in-favor comment; looks to me like we should close (and #34121). (I was just trying to help out with easy filed issues and don't personally have any particular interest in -Z vs. logging.)

@zackmdavis zackmdavis closed this Jan 3, 2017
@zackmdavis zackmdavis deleted the remove_Z_input_stats branch January 13, 2018 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants