Skip to content

Use new io in print and println macroses #23206

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 15, 2015
Merged

Use new io in print and println macroses #23206

merged 1 commit into from
Mar 15, 2015

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Mar 8, 2015

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
Copy link
Contributor

hauleth commented Mar 9, 2015

👍

@alexcrichton
Copy link
Member

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:

cc @aturon, do you have thoughts on this?

@hauleth
Copy link
Contributor

hauleth commented Mar 9, 2015

@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
Copy link
Member Author

nagisa commented Mar 9, 2015

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

@nagisa nagisa force-pushed the print-io branch 2 times, most recently from a872254 to 040a97e Compare March 9, 2015 19:26
@alexcrichton
Copy link
Member

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
Copy link
Collaborator

bors commented Mar 13, 2015

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

@nagisa nagisa force-pushed the print-io branch 2 times, most recently from 045ba62 to 1032da3 Compare March 13, 2015 22:59
alexcrichton
alexcrichton reviewed Mar 13, 2015
View reviewed changes
src/libstd/macros.rs
@@ -83,7 +84,8 @@ macro_rules! print {
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
macro_rules! println {
($($arg:tt)*) => ($crate::old_io::stdio::println_args(format_args!($($arg)*)))
($fmt:expr) => (print!(concat!($fmt, "\n")));
($fmt:expr, $($arg:tt)*) => (print!(concat!($fmt, "\n"), $($arg)*));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@alexcrichton
Copy link
Member

@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 nagisa force-pushed the print-io branch 3 times, most recently from ded8dd9 to 38048f8 Compare March 14, 2015 11:33
nagisa
nagisa reviewed Mar 14, 2015
View reviewed changes
src/libtest/stats.rs
}
map
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alexcrichton
Copy link
Member

@bors: r+ 38048f8c0c794ca80e754af427c9f1c0d60ee3ce

Thanks!

@bors
Copy link
Collaborator

bors commented Mar 14, 2015

⌛ Testing commit 38048f8 with merge a17aed7...

@bors
Copy link
Collaborator

bors commented Mar 14, 2015

💔 Test failed - auto-mac-64-opt

@nagisa
Copy link
Member Author

nagisa commented Mar 14, 2015

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

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 15, 2015

@bors r=alexcrichton 6e92f05

tamird
tamird reviewed Mar 15, 2015
View reviewed changes
src/libstd/io/stdio.rs
@@ -18,6 +19,13 @@ use io::{self, BufReader, LineWriter};
use sync::{Arc, Mutex, MutexGuard};
use sys::stdio;

/// Stdout used by print! and println! macroses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

macros?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

bors commented Mar 15, 2015

⌛ Testing commit 6e92f05 with merge c62ae87...

bors added a commit that referenced this pull request Mar 15, 2015
@bors
Auto merge of #23206 - nagisa:print-io, r=alexcrichton
c62ae87 
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?
@bors
Copy link
Collaborator

bors commented Mar 15, 2015

☀️ 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

@bors bors merged commit 6e92f05 into rust-lang:master Mar 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nagisa @hauleth @alexcrichton @bors @tamird