Use new io in print and println macroses

[nagisa]

r? @alexcrichton or @aturon

This still needs to somehow figure out how to avoid unstable warnings arising from the use of unstable functions. I tried to use #[allow_internal_unstable] but it still spits out warnings as far as I can see. @huonw (I think you implemented it) does #[allow_internal_unstable] not work for some reason or am I using it incorrectly?

[hauleth]

👍

[alexcrichton]

I think this should rather call a function in std::io itself to reduce code bloat at call sites, something like this should do the trick:

// in src/libstd/io/mod.rs
#[doc(hidden)]
pub mod __print {
    pub fn println(args: &fmt::Arguments) { ... }
    pub fn print(args: &fmt::Arguments) { ... }
}

We may want to also hold off on this for now for a little bit, I have a few reservations:

• Resolve at_exit strategy after std::thread lands #20012 and it causing New IO stdout is not flushed at program exit #23191 is worrisome
• This breaks capturing of output in tests. I don't think that any "official" decision has been made to let this slide. It will certainly start polluting the output of stdtest probably a little more than it already is.

cc @aturon, do you have thoughts on this?

[hauleth]

@alexcrichton maybe print could call flush and that will fix issues from #23191. It can be slower than "raw" with buffering, but IMHO it would not be big issue.

[nagisa]

@alexcrichton regarding the bloat: we are talking about println(format_args!(…)) vs stdout().write_fmt(format_args!(…)) here. Sure, the latter has some additional overhead, but only a little (calling println which println calls vs overhead calling stdout() + Write::write_fmt).

Currently, as patch is now, this comes out to 17 insns (old) vs 30 insns (new) when optimised and ~50 insns (old) vs ~100 insns (new) without optimisations. Most of the additional overhead in the new version comes from multiple invocations of format_args!. This can easily be avoided/remedied by calling write_fmt directly and concat!ing the format string with a newline smartly (20 insns optimised and ~70 insns not optimised).

Regarding other points, I’m fine with holding the PR off until flushing at exit works.

@hauleth I’d rather see at_exit() working instead. This is not time critical PR and there’s no need to resort to hacks yet.

EDIT: err, I didn’t account for all the drop glue in my instruction calculations.

[alexcrichton]

EDIT: err, I didn’t account for all the drop glue in my instruction calculations.

Ah yeah I've found in the past that this introduces quite a bit more code to deal with the owned Stdout value along with the owned io::Result<()> value. As a measurement of 1k println!("Hello {}", "world") it is 715K for me before this change and 1.2M after this change. The macro here also has a much higher compile time for the test case.

Basically I do agree that the differences are pretty minor, but for a super heavily used macro like println! LLVM will definitely benefit from not having to inline anything as well as not having to deal with drop glue.

[bors]

☔ The latest upstream changes (presumably #23292) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Nice!

[alexcrichton]

@nagisa I think that for now we may wish to continuing capturing the output of tests just to preserve that compatibility temporarily. I ended up adding set_panic to support the compiler's use case as well. Could you add a quick shim (similar to that) for stdout as well? Other than that I think this is good to go now that #23267 should land soon.

[nagisa]

This code is not used anywhere, so I took liberty to get rid of it.

[alexcrichton]

@bors: r+ 38048f8c0c794ca80e754af427c9f1c0d60ee3ce

Thanks!

[bors]

⌛ Testing commit 38048f8 with merge a17aed7...

[bors]

💔 Test failed - auto-mac-64-opt

[nagisa]

I believe that test is not applicable anymore, hence the test goes away.

[alexcrichton]

@bors: r+

[bors]

@bors r=alexcrichton 6e92f05

[tamird]

macros?

[nagisa]

Since this code is internal and docstring won’t appear in any public-facing documentation, I believe it is not worth updating this unless this bounces.

[bors]

⌛ Testing commit 6e92f05 with merge c62ae87...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt