Stream: t-compiler

Topic: #54818 weekly meeting 2019-01-24


pnkfelix (Jan 24 2019 at 13:38, on Zulip):

Hi @T-compiler/meeting ; our meeting will start in about 80 minutes. I'm just going to take pre-meeting triage notes here in the meantime.

pnkfelix (Jan 24 2019 at 13:41, on Zulip):

first up: P-high issues

pnkfelix (Jan 24 2019 at 13:42, on Zulip):

there are 9 on the list but a bunch are tagged as A-NLL.

pnkfelix (Jan 24 2019 at 13:43, on Zulip):

All the ones tagged with A-NLL have assignees. I haven't yet reviewed the notes from last night's NLL triage meeting but I'm going to assume for now that all the P-high issues tagged with A-NLL are under control from our point of view.

pnkfelix (Jan 24 2019 at 13:43, on Zulip):

So, first up is: "Compiler crash" #57843 (filed two days ago)

pnkfelix (Jan 24 2019 at 13:44, on Zulip):

its a (stable-to-beta) regression. Its already tagged as P-high.

pnkfelix (Jan 24 2019 at 13:45, on Zulip):

I'll assign to self for initial investigation.

pnkfelix (Jan 24 2019 at 13:47, on Zulip):

(The good news is that someone already bisected it and it was injected by one of #55517 #56507 #57282. Of those, I think #55517 is most suspicious.)

pnkfelix (Jan 24 2019 at 13:47, on Zulip):

next: "ICE on nightly when dereferencing boxed Iterator trait object" #57673

pnkfelix (Jan 24 2019 at 13:48, on Zulip):

I worked on this and posted a "fix", PR #57835. We'll discuss it more when we talk about beta-backports.

pnkfelix (Jan 24 2019 at 13:49, on Zulip):

next: "Regression in trait bounds that are redundant with associated type's HRTB" #57639

pnkfelix (Jan 24 2019 at 13:51, on Zulip):

niko has some notes and discussion on the ticket. I mostly am curious about @David Tolnay most recent comment, "If it didn't appear in crater, and there isn't an easy fix, it seems fine to postpone until the trait solver is better prepared to support this case" -- how long are we talking about being willing to wait?

pnkfelix (Jan 24 2019 at 13:52, on Zulip):

I would assume the hope is that we would find a fix before this bug leaks out into beta? I'll post that Q (aimed at @nikomatsakis ) on the ticket itself.

pnkfelix (Jan 24 2019 at 13:54, on Zulip):

oh the bug has already leaked out onto beta

pnkfelix (Jan 24 2019 at 13:56, on Zulip):

next: "[NLL] Bad higher ranked subtype error" #57374

pnkfelix (Jan 24 2019 at 13:56, on Zulip):

oh sorry I forgot I'm skipping A-NLL stuff. Never mind that one. :smiley:

pnkfelix (Jan 24 2019 at 13:57, on Zulip):

next: "Confusing error message associated with universe transition: 'one type is more general than the other'" #57362

pnkfelix (Jan 24 2019 at 13:58, on Zulip):

There hasn't been any activity since niko left notes 9 days ago. @lqd : you assigned yourself 8 days ago, do you have any updates here? Feel free to leave comment on ticket if so.

pnkfelix (Jan 24 2019 at 13:59, on Zulip):

(we also need to ask ourselves if his is indeed P-high. It probably is, but I want to make sure we at least consider the question.)

pnkfelix (Jan 24 2019 at 14:00, on Zulip):

next: "PhantomData fields in repr(C) structs change ABI on aarch64" #56877

lqd (Jan 24 2019 at 14:01, on Zulip):

@pnkfelix yeah I'm working on it: fixing the immediate problem isn't enough, the error messages need to be a bit clearer or it wouldn't improve these errors enough, so niko gave me the couple pointers I needed to be unblocked yesterday. The activity is in the "#57362 confusing errors after universe transition" topic

pnkfelix (Jan 24 2019 at 14:02, on Zulip):

@lqd ah I should have thought to look for a stream with the issue number. thanks!

lqd (Jan 24 2019 at 14:02, on Zulip):

(I'll update the issue later today to mention the status, but probably couldn't do so before the team meeting)

pnkfelix (Jan 24 2019 at 14:04, on Zulip):

for #56877, there's been a fair amount of recent discussion.

pnkfelix (Jan 24 2019 at 14:07, on Zulip):

it looks like @nikomatsakis has identified two different strategies for a change to the logic that decides whether (for FFI/ABI purposes) whether a type definition is a homogeneous aggregate.

nikomatsakis (Jan 24 2019 at 14:08, on Zulip):

yes. @eddyb had a particularly way that they wanted me to implement the change, which I will hopefully do today

pnkfelix (Jan 24 2019 at 14:09, on Zulip):

i left a couple comments.

pnkfelix (Jan 24 2019 at 14:09, on Zulip):

the rest are all NLL things. (Oh god I really need to pick back up #56254)

pnkfelix (Jan 24 2019 at 14:10, on Zulip):

so okay that's all the P-high things that aren't tagged A-NLL.

pnkfelix (Jan 24 2019 at 14:11, on Zulip):

next up: beta-nominated PRs. I'll jump in now and add T-compiler tags to the ones that are obviously for us.

pnkfelix (Jan 24 2019 at 14:12, on Zulip):

first up: "Fix issue 57762" #57840. This is a patch to librustc_codegen_llvm

pnkfelix (Jan 24 2019 at 14:12, on Zulip):

its just forcing us to use a more recent version of LLVM.

pnkfelix (Jan 24 2019 at 14:13, on Zulip):

I see ... no reason to not beta-accept this. The only drawback is that it might cause problems for some people trying to build their own rustc with an older (system) LLVM, right? (see e.g. #57762)

pnkfelix (Jan 24 2019 at 14:14, on Zulip):

Well I'll wait until the actual meeting to add the beta-accepted tag, to ensure attending members get a chance to voice an objection.

pnkfelix (Jan 24 2019 at 14:14, on Zulip):

next: "typeck: remove leaky nested probe during trait object method resolution" #57835

pnkfelix (Jan 24 2019 at 14:17, on Zulip):

this is my patch to fix #57673. @Ariel Ben-Yehuda has noted that this may not be quite right because it will cause a conditional deref to be forced to occur, which may "pollute the other candidates". I'm not 100% sure how to interpret "pollute" there; it could mean "we will fail to consider candidates that we should." But it might also mean "we may consider candidates that we shouldn't."

pnkfelix (Jan 24 2019 at 14:18, on Zulip):

If "pollute" can mean "we may consider candidates that we shouldn't", then I would not be in favor of a backport.

pnkfelix (Jan 24 2019 at 14:18, on Zulip):

@Ariel Ben-Yehuda @nikomatsakis ^ can you clarify this detail?

pnkfelix (Jan 24 2019 at 14:19, on Zulip):

if "pollute" solely means "we will fail to consider candidates that we should be considering") then I am in favor of a backport, based on a rough impression that I don't know when @Ariel Ben-Yehuda will get around to supplying a proper fix.

pnkfelix (Jan 24 2019 at 14:20, on Zulip):

Also: if someone can supply a concrete example showing the case @Ariel Ben-Yehuda is concerned about, then I might be able to come up with a better patch on my own...

pnkfelix (Jan 24 2019 at 14:20, on Zulip):

next: "Fix typo bug in DepGraph::try_mark_green()." #57698

pnkfelix (Jan 24 2019 at 14:20, on Zulip):

omg

pnkfelix (Jan 24 2019 at 14:20, on Zulip):

dep_dep_dep_dep_node

pnkfelix (Jan 24 2019 at 14:21, on Zulip):

anyway, I see no reason not to backport this. I'll wait until the actual meeting to add tags though.

pnkfelix (Jan 24 2019 at 14:21, on Zulip):

okay that's all the T-compiler PR's nominated for beta backport.

pnkfelix (Jan 24 2019 at 14:22, on Zulip):

the PR #57840 is also nominated for a stable backport.

nikomatsakis (Jan 24 2019 at 14:22, on Zulip):

@Ariel Ben-Yehuda @nikomatsakis ^ can you clarify this detail?

to be quite honest I haven't had any time to really look into that PR or think about it (I hadn't even seen Ariel's comment, for example)

nikomatsakis (Jan 24 2019 at 14:23, on Zulip):

I should probably prioritize that

pnkfelix (Jan 24 2019 at 14:23, on Zulip):

@Ariel Ben-Yehuda referred to something called a "conditional deref"

pnkfelix (Jan 24 2019 at 14:23, on Zulip):

I don't know when that would arise. Something with autoref?

pnkfelix (Jan 24 2019 at 14:24, on Zulip):

the PR #57840 is also nominated for a stable backport.

(i ... dont know what to think about a backport to stable for that. It doesn't seem worth a point release on its own. But if we're already doing a point release for other reasons ... ?)

pnkfelix (Jan 24 2019 at 14:25, on Zulip):

okay. Next up is the list of stable-to-beta regressions.

pnkfelix (Jan 24 2019 at 14:25, on Zulip):

we've already talked about all of the T-compiler ones since they are marked as P-high

pnkfelix (Jan 24 2019 at 14:25, on Zulip):

next is stable-to-nightly regressions

pnkfelix (Jan 24 2019 at 14:26, on Zulip):

only one is potentially T-compiler and not already tagged as P-high

pnkfelix (Jan 24 2019 at 14:26, on Zulip):

namely: "Project fails to compile on latest stable and nightly (hang/stackoverflow)" #57735

pnkfelix (Jan 24 2019 at 14:27, on Zulip):

worth noting that on an updated nightly the reporter indicates that on the platform where they can reproduce, the compilation succeeds but takes a "long time"

pnkfelix (Jan 24 2019 at 14:29, on Zulip):

reporter also indicates that "nightly-x86_64-apple-darwin (rustc 1.33.0 nightly c2d381d 2019-01-10)" does not suffer from the problem. (Or at least that's how I interpret their comment)

pnkfelix (Jan 24 2019 at 14:31, on Zulip):

okay well I'll tag this as P-high and assign to self.

pnkfelix (Jan 24 2019 at 14:35, on Zulip):

next: PR waiting for our team (and other teams): "Automatically open an issue when a tool breaks" #56951

pnkfelix (Jan 24 2019 at 14:37, on Zulip):

actually, skimming over the checkbox list, looks to me like everyone on T-compiler has already checked off their box

pnkfelix (Jan 24 2019 at 14:39, on Zulip):

perhaps a faux pas, but posted comment pinging the people whose checkboxes are unchecked

pnkfelix (Jan 24 2019 at 14:43, on Zulip):

I left PR #57214 on the I-nominated list. I'll keep it here until we've resolved the schedule for the all-hands

pnkfelix (Jan 24 2019 at 14:56, on Zulip):

Okay so I think I've finished the pretriage.

pnkfelix (Jan 24 2019 at 14:57, on Zulip):

with 3 minutes left until @T-compiler/meeting should show up. :smiley:

mw (Jan 24 2019 at 14:57, on Zulip):

:wave:

nagisa (Jan 24 2019 at 15:02, on Zulip):

Is it 2 minutes past 17 now?

nagisa (Jan 24 2019 at 15:02, on Zulip):

/me reads the backlog

pnkfelix (Jan 24 2019 at 15:03, on Zulip):

yeah sorry

pnkfelix (Jan 24 2019 at 15:03, on Zulip):

/me got distracted closing Open Windows

pnkfelix (Jan 24 2019 at 15:04, on Zulip):

I did pre-mtg triage and there isn't too too much to report.

pnkfelix (Jan 24 2019 at 15:05, on Zulip):

I would like to put some sort of triage comment into #57362

nikomatsakis (Jan 24 2019 at 15:05, on Zulip):

"in progress"

nikomatsakis (Jan 24 2019 at 15:05, on Zulip):

I think we're getting close

nikomatsakis (Jan 24 2019 at 15:05, on Zulip):

I can write an update

pnkfelix (Jan 24 2019 at 15:06, on Zulip):

okay I'll leave that to @nikomatsakis or @lqd to take care of

pnkfelix (Jan 24 2019 at 15:06, on Zulip):

I don't think any of the other P-high issues (that aren't otherwise beta-nominated) warrant discussion now

pnkfelix (Jan 24 2019 at 15:07, on Zulip):

so, moving onto beta-nominations

pnkfelix (Jan 24 2019 at 15:07, on Zulip):

First up: "Fix issue 57762" #57840 is nominated for backport to both beta and stable.

pnkfelix (Jan 24 2019 at 15:08, on Zulip):

It is just updating a version check to ensure that when you link to a system LLVM, you need to use version >=8

pnkfelix (Jan 24 2019 at 15:08, on Zulip):

so: I think this is fine for backport to beta

nikomatsakis (Jan 24 2019 at 15:08, on Zulip):

okay I'll leave that to @nikomatsakis or @lqd to take care of

added a comment

pnkfelix (Jan 24 2019 at 15:08, on Zulip):

so I'm going to tag #57840 as beta-accepted. If someone objects before end of meeting, we can discuss.

pnkfelix (Jan 24 2019 at 15:09, on Zulip):

what about the backport to stable, though?

pnkfelix (Jan 24 2019 at 15:09, on Zulip):

I don't personally think this is worth a point release on its own

Nikita Popov (Jan 24 2019 at 15:10, on Zulip):

For context, this fixes rustc built against LLVM 7.0.1, which is a problem a few distros ran into recently. It's probably good to backport to beta as a courtesy to distributors, but a stable backport seems superfluous. They are perfectly capable of patching code themselves.

pnkfelix (Jan 24 2019 at 15:11, on Zulip):

so the main thing I'm wonderng

pnkfelix (Jan 24 2019 at 15:11, on Zulip):

if we were to happen to have a point release for other reasons

pnkfelix (Jan 24 2019 at 15:11, on Zulip):

would we want this change in there in that case?

QuietMisdreavus (Jan 24 2019 at 15:11, on Zulip):

there's another stable-accepted PR for rustdoc with a similar "not worth it on its own but could be included with others" rationale

mw (Jan 24 2019 at 15:11, on Zulip):

would we want this change in there in that case?

seems small enough

pnkfelix (Jan 24 2019 at 15:11, on Zulip):

I'm essentially wondering if it makes sense to still backport this to stable branch but somehow indicate "we're not expected to get a point release based on this"

QuietMisdreavus (Jan 24 2019 at 15:12, on Zulip):

core/release teams still need to decide on the release, even if a PR is stable-accepted

pnkfelix (Jan 24 2019 at 15:12, on Zulip):

(Or is that a bad idea, in terms of some philosophy that the tip of the stable branch needs to match the stable release itself?)

QuietMisdreavus (Jan 24 2019 at 15:12, on Zulip):

at least, that was my takeaway from talking with pietro about the rustdoc PR

pnkfelix (Jan 24 2019 at 15:12, on Zulip):

Hmm. Okay. So maybe we should just be evaluating risk/effort on its own for our team on this PR on its own?

pnkfelix (Jan 24 2019 at 15:12, on Zulip):

and not worry about the broader questions ?

QuietMisdreavus (Jan 24 2019 at 15:13, on Zulip):

basically, yeah

pnkfelix (Jan 24 2019 at 15:13, on Zulip):

(in which case, I would probably be in favor of marking as stable-accepted ?)

Wesley Wiser (Jan 24 2019 at 15:13, on Zulip):

(Or is that a bad idea, in terms of some philosophy that the tip of the stable branch needs to match the stable release itself?)

I feel like this is what tags are for.

QuietMisdreavus (Jan 24 2019 at 15:13, on Zulip):

effectively, marking something stable-accepted means "this is worth including in a hypothetical point release, should one happen"

pnkfelix (Jan 24 2019 at 15:14, on Zulip):

okay then, I guess my current inclination is to then mark this as stable-accepted but leave a comment saying that this PR on its own does not warrant a point release

QuietMisdreavus (Jan 24 2019 at 15:14, on Zulip):

as opposed to beta-accepted's meaning of "please move this to beta"

QuietMisdreavus (Jan 24 2019 at 15:14, on Zulip):

it's worth mentioning something in the release team's channel on discord, but otherwise yeah

mw (Jan 24 2019 at 15:14, on Zulip):

a meta question: if something is beta accepted, who is doing the backporting? in the past I often did the backport if I was the original author of the PR, but more recently there seem to be quite a few people who are happy to provide this service. Is this codified somewhere? I.e. do we have designated "backporters"?

QuietMisdreavus (Jan 24 2019 at 15:14, on Zulip):

afaik release team batches up beta backport PRs?

pnkfelix (Jan 24 2019 at 15:15, on Zulip):

Yeah in my experience the release team often seems to take care of beta backports before I get around to doing it, at least.

mw (Jan 24 2019 at 15:16, on Zulip):

that's my experience too. the question is: do I have to keep up-to-date with backporting progress?

pnkfelix (Jan 24 2019 at 15:16, on Zulip):

@mw it seems like it might be a good idea to at least keep tabs on it, but maybe that's more a job for me or whomever else is running the compiler meeting each week...?

nikomatsakis (Jan 24 2019 at 15:17, on Zulip):

I feel like it would make sense to have some kind of 'release-compiler sync'

nikomatsakis (Jan 24 2019 at 15:17, on Zulip):

not necessarily a meeting

mw (Jan 24 2019 at 15:17, on Zulip):

it probably depends on how hard it is to keep track of this

nikomatsakis (Jan 24 2019 at 15:17, on Zulip):

but some clearer way to communicate with one another when things need doing

nikomatsakis (Jan 24 2019 at 15:17, on Zulip):

which might be something we could check in on during these meetings, for example

Vadim Petrochenkov (Jan 24 2019 at 15:17, on Zulip):

do I have to keep up-to-date with backporting progress?

No, from what I've seen.
If something happens (e.g. the patch doesn't apply cleanly), then you are pinged.

nikomatsakis (Jan 24 2019 at 15:17, on Zulip):

I mean I guess we sort of do, via the existing labels

pnkfelix (Jan 24 2019 at 15:18, on Zulip):

anyway, just to keep things humming along here (though I don't mind continuing this parallel conversation regarding backport policy), the next beta nomination is "typeck: remove leaky nested probe during trait object method resolution" #57835

nikomatsakis (Jan 24 2019 at 15:18, on Zulip):

it seems like we're still debating the fix itself at this point, right?

pnkfelix (Jan 24 2019 at 15:18, on Zulip):

well, @Ariel Ben-Yehuda approved it for nightly

pnkfelix (Jan 24 2019 at 15:18, on Zulip):

so the main Q is whether we want to wait to see further work before doing any backport at all

pnkfelix (Jan 24 2019 at 15:19, on Zulip):

or if it would still make sense to backport this

pnkfelix (Jan 24 2019 at 15:19, on Zulip):

and, if something else comes up, we can independently consider whether to backport that too

nikomatsakis (Jan 24 2019 at 15:19, on Zulip):

oh, I see, ok

nikomatsakis (Jan 24 2019 at 15:19, on Zulip):

I guess so

nikomatsakis (Jan 24 2019 at 15:19, on Zulip):

we could sit on it for a week

nikomatsakis (Jan 24 2019 at 15:20, on Zulip):

(to see what happens when it hits nightly)

pnkfelix (Jan 24 2019 at 15:20, on Zulip):

the backstory here, y'all, is that there's this ICE and ariel has a comment saying that the proposed fix is not ideal

pnkfelix (Jan 24 2019 at 15:20, on Zulip):

but I do not completely understand the exact repercussions of the scenario that @Ariel Ben-Yehuda outlined in their comment

pnkfelix (Jan 24 2019 at 15:20, on Zulip):

and I am also a little worried about how much free time @Ariel Ben-Yehuda has to enlighten me about it

pnkfelix (Jan 24 2019 at 15:21, on Zulip):

having said that

pnkfelix (Jan 24 2019 at 15:21, on Zulip):

lets wait a week!

pnkfelix (Jan 24 2019 at 15:21, on Zulip):

:)

nagisa (Jan 24 2019 at 15:23, on Zulip):

Note that the next train rollover is in march 1st. So a lot of time.

pnkfelix (Jan 24 2019 at 15:23, on Zulip):

okay, next: "Fix typo bug in DepGraph::try_mark_green()." #57698

pnkfelix (Jan 24 2019 at 15:23, on Zulip):

we should back_backport this

pnkfelix (Jan 24 2019 at 15:24, on Zulip):

oh wow, @nagisa, what's your objection here?

pnkfelix (Jan 24 2019 at 15:24, on Zulip):

hmm, mixed signals

nagisa (Jan 24 2019 at 15:24, on Zulip):

I’m just making things easy for people to click.

pnkfelix (Jan 24 2019 at 15:24, on Zulip):

oh. hmm.

nikomatsakis (Jan 24 2019 at 15:24, on Zulip):

nagisa adds both :) then removes one

pnkfelix (Jan 24 2019 at 15:24, on Zulip):

okay I see.

pnkfelix (Jan 24 2019 at 15:25, on Zulip):

I'll maybe do that myself

pnkfelix (Jan 24 2019 at 15:25, on Zulip):

so that I know what's going on

varkor (Jan 24 2019 at 15:25, on Zulip):

seems we could do with a Zulip triage bot

pnkfelix (Jan 24 2019 at 15:25, on Zulip):

okay that's all the beta-nominations for T-compiler

pnkfelix (Jan 24 2019 at 15:26, on Zulip):

that's all the standing items for the triage meeting

pnkfelix (Jan 24 2019 at 15:27, on Zulip):

but maybe we could all move over to the all hands planning topic ?

pnkfelix (Jan 24 2019 at 15:27, on Zulip):

(or maybe I should first ask if anyone has any items they want to discuss here that wouldn't fit into the all hands planning topic.)

mw (Jan 24 2019 at 15:28, on Zulip):

I have one thing to discuss: https://github.com/rust-lang/cargo/pull/6564

mw (Jan 24 2019 at 15:28, on Zulip):

it's about making incr. comp. the default for --release builds

mw (Jan 24 2019 at 15:28, on Zulip):

since we have ThinLTO + incr. comp., we might want to do this

mw (Jan 24 2019 at 15:28, on Zulip):

i.e. generated code has very good runtime performance

pnkfelix (Jan 24 2019 at 15:28, on Zulip):

what do you think this means "Gosh, I'm a little sad that not having had time for a try feature could now be a part of the geology of master :P." ?

pnkfelix (Jan 24 2019 at 15:29, on Zulip):

are they concerned that it hasn't gotten enough testing?

nagisa (Jan 24 2019 at 15:29, on Zulip):

There is still this. Is it in any way applicable to this change?

nagisa (Jan 24 2019 at 15:29, on Zulip):

(I believe codegen-units+thinlto is already a default, perhaps turning incremental on changes something here?)

nikomatsakis (Jan 24 2019 at 15:30, on Zulip):

I'm basically in favor of this. My only concern is that there remain some cases (notably, servo's script crate) where incremental is quite a bit slower. But I guess that's essentially an orthogonal concern, as long as it's not a widespread thing (we should work on it though)

mw (Jan 24 2019 at 15:30, on Zulip):

performance overall would be a bit worse again, compared to non-incrmental builds

Zoxc (Jan 24 2019 at 15:31, on Zulip):

@mw I feel like --release is a misleading name, since you'd probably want a full LTO, 1 CGU, non-incremental build for actual releases

mw (Jan 24 2019 at 15:31, on Zulip):

my suggestion is to enable this temporarily so we get some numbers from lolbench

mw (Jan 24 2019 at 15:31, on Zulip):

@Zoxc yes, I want a dev-opt profile

mw (Jan 24 2019 at 15:31, on Zulip):

but the cargo team seems to not want that :)

nikomatsakis (Jan 24 2019 at 15:31, on Zulip):

This has been a perennial topic :/

Zoxc (Jan 24 2019 at 15:32, on Zulip):

I'd also like a fast profile too. Generating debug info is slow =P

nikomatsakis (Jan 24 2019 at 15:32, on Zulip):

I still favor dev-opt

nikomatsakis (Jan 24 2019 at 15:32, on Zulip):

hmm

mw (Jan 24 2019 at 15:32, on Zulip):

as far as I understand it: if we enable this now and revert in a few weeks, no actual release would be affected

nikomatsakis (Jan 24 2019 at 15:32, on Zulip):

it feels like there is maybe a mildly bigger conversation than just yay/nay here?

mw (Jan 24 2019 at 15:33, on Zulip):

but we'd still get some numbers from lolbench about the runtime performance of incrementally compiled release builds

pnkfelix (Jan 24 2019 at 15:33, on Zulip):

what do you think this means "Gosh, I'm a little sad that not having had time for a try feature could now be a part of the geology of master :P." ?

(Oh oh, I see now: a "try feature" is talking about a weakness in lolbench)

mw (Jan 24 2019 at 15:33, on Zulip):

we can also make it a topic for discussion at the all hands

mw (Jan 24 2019 at 15:34, on Zulip):

and pester the cargo people for dev-opt there :P

pnkfelix (Jan 24 2019 at 15:34, on Zulip):

I kind of like the idea of turning it on with the understanding that we will turn it off very eagerly

Zoxc (Jan 24 2019 at 15:34, on Zulip):

But --release as we have it now effectively is dev-opt, so I'd be fine with having incremental ThinLTO on by default

nagisa (Jan 24 2019 at 15:34, on Zulip):

well, add a --publish.

mw (Jan 24 2019 at 15:35, on Zulip):

@Zoxc, yes but people only recently have gotten around to doing their benchmarks with --release

nagisa (Jan 24 2019 at 15:35, on Zulip):

now that does really drive it home this is the release-release configuration

mw (Jan 24 2019 at 15:35, on Zulip):

I don't want to increase the confusion :)

Zoxc (Jan 24 2019 at 15:35, on Zulip):

And I expect users who need better performance manually tweak cargo options (which is currently needed for CGUs, panic=abort and full LTOs anyway)

mw (Jan 24 2019 at 15:36, on Zulip):

I'd hope so, yeah

mw (Jan 24 2019 at 15:36, on Zulip):

we should write a "compiling your Rust code" book

mw (Jan 24 2019 at 15:36, on Zulip):

that goes into detail about the options available and their consequences

Zoxc (Jan 24 2019 at 15:36, on Zulip):

I'd also expect incremental ThinLTO's runtime performance to improve over time, making it matter less

pnkfelix (Jan 24 2019 at 15:36, on Zulip):

can we compare "make this change now" vs "decide at all-hands whether to make the change" ?

pnkfelix (Jan 24 2019 at 15:37, on Zulip):

I think we'd be better off making the change now so that we (potentially) have feedback data to discuss at all-hands

nagisa (Jan 24 2019 at 15:37, on Zulip):

making this change now gives us a month and a week or so. Deciding on it during all hands gives us 3 weeks for experimentation and potential revert.

mw (Jan 24 2019 at 15:37, on Zulip):

we could enable now and collect data until the all hands

pnkfelix (Jan 24 2019 at 15:37, on Zulip):

maybe that's even better: Enable now. Uncondtionally revert shortly before the all-hands.

mw (Jan 24 2019 at 15:37, on Zulip):

i.e. lolbench profits from testing multiple nightlies

pnkfelix (Jan 24 2019 at 15:38, on Zulip):

then we'd get feedback about both directions

pnkfelix (Jan 24 2019 at 15:38, on Zulip):

(since people don't complain about improvements)

mw (Jan 24 2019 at 15:38, on Zulip):

@pnkfelix interesting

mw (Jan 24 2019 at 15:38, on Zulip):

a jedi trick

Zoxc (Jan 24 2019 at 15:38, on Zulip):

I also wonder at what point we should enabled parallel computation in rustc. I also want to test that on nighties and turn it off later

mw (Jan 24 2019 at 15:39, on Zulip):

@Zoxc we should open an issue that lists the blockers for that

Zoxc (Jan 24 2019 at 15:40, on Zulip):

We'd have to land https://github.com/rust-lang/rust/pull/56946 first, ping @nikomatsakis ;) I don't think there's any other blockers though?

mw (Jan 24 2019 at 15:40, on Zulip):

how is the test suite doing these days?

nagisa (Jan 24 2019 at 15:40, on Zulip):

@pnkfelix we should review the optimize(foo) attribute implementation at some point

pnkfelix (Jan 24 2019 at 15:41, on Zulip):

how is the test suite doing these days?

what do you mean by this?

mw (Jan 24 2019 at 15:41, on Zulip):

@Zoxc although, maybe let's make a decision about the other topic first

mw (Jan 24 2019 at 15:41, on Zulip):

@pnkfelix that's re: parallel queries

pnkfelix (Jan 24 2019 at 15:41, on Zulip):

@nagisa yes

pnkfelix (Jan 24 2019 at 15:42, on Zulip):

#55641 is currently tagged as waiting-on-author, lets see

pnkfelix (Jan 24 2019 at 15:42, on Zulip):

that is probably a mis-tag at this point, I'm guessing.

Zoxc (Jan 24 2019 at 15:42, on Zulip):

@mw How bad is the runtime performance regressions with incremental ThinLTO?

mw (Jan 24 2019 at 15:43, on Zulip):

so, judging from the responses so far, it looks like I can tell @Alex Crichton that we are OK with doing @pnkfelix's proposal

mw (Jan 24 2019 at 15:43, on Zulip):

that is, enable now until just before the all hands

pnkfelix (Jan 24 2019 at 15:43, on Zulip):

that is probably a mis-tag at this point, I'm guessing.

(yeah that tag dates from November. I'll tag it back to waiting on review)

mw (Jan 24 2019 at 15:43, on Zulip):

@Zoxc these are the best numbers we have so far: https://github.com/rust-lang/rust/pull/56678

Zoxc (Jan 24 2019 at 15:44, on Zulip):

@mw Also how does incremental ThinLTO work, is there some LLVM docs to link to?

nagisa (Jan 24 2019 at 15:44, on Zulip):

@pnkfelix I want to just have a general r=me with compile issues fixed, at which point I’ll just fix them, because rebasing the PR gets really boring very quick when the conflict is always in the area where the stability attributes are defined.

mw (Jan 24 2019 at 15:45, on Zulip):

@Zoxc the non-incremental builds indicate how much slower the compiler got due to being compiled incrementally

pnkfelix (Jan 24 2019 at 15:45, on Zulip):

@nagisa sure. I'll do a re-review pronto. I was trusting the tags and didn't see your comments.

mw (Jan 24 2019 at 15:45, on Zulip):

regression looks to be < 1%, except for edge cases

mw (Jan 24 2019 at 15:46, on Zulip):

well, more like 0.5 - 3%

mw (Jan 24 2019 at 15:46, on Zulip):

but if some critical inlining is missed, performance is destroyed (e.g. + 25% for hello workd)

mw (Jan 24 2019 at 15:47, on Zulip):

we've seen similar things when making ThinLTO the default for regular builds

Zoxc (Jan 24 2019 at 15:48, on Zulip):

Do you know why hello-world is much slower? Is the rustc startup cost just worse? Can we avoid that with #[inline]?

mw (Jan 24 2019 at 15:49, on Zulip):

@Zoxc inlining the appropriate functions would probably help. but it doesn't really matter. the compiler is just used as a benchmark here

mw (Jan 24 2019 at 15:50, on Zulip):

@Zoxc https://github.com/rust-lang/rust/pull/53673 has some discussion on how incr. ThinLTO is implemented in rustc

mw (Jan 24 2019 at 15:50, on Zulip):

LLD does it similarly

mw (Jan 24 2019 at 15:51, on Zulip):

@Zoxc http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html has some background on it too

Zoxc (Jan 24 2019 at 15:53, on Zulip):

I'm just wondering if #[inline] actually has an effect, or if incremental ThinLTO actually avoids inlining in other to be more incremental.

mw (Jan 24 2019 at 15:53, on Zulip):

#[inline] is respected by release builds, even in incremental mode

Zoxc (Jan 24 2019 at 15:55, on Zulip):

25% seems pretty bad though, but I'd be fine with having it temporarily enabled to find out of if that's more of a fluke or more common

mw (Jan 24 2019 at 15:55, on Zulip):

yeah

mw (Jan 24 2019 at 15:56, on Zulip):

I think the situation is very similar to when we made ThinLTO the default a while ago

mw (Jan 24 2019 at 15:56, on Zulip):

performance was "roughly" the same but some microbenchmarks tanked :)

Zoxc (Jan 24 2019 at 15:57, on Zulip):

hello-world is probably quite representative of our test suite though =P

mw (Jan 24 2019 at 15:58, on Zulip):

that's true, but the default would just be for cargo, so the compiler would not be affected

mw (Jan 24 2019 at 15:58, on Zulip):

or at least, we would keep it opt-in

Zoxc (Jan 24 2019 at 15:59, on Zulip):

I'd probably use it for actual development though =P

mw (Jan 24 2019 at 15:59, on Zulip):

yes, that's one case where many cpu cores should help mitigate the problem :)

mw (Jan 24 2019 at 16:00, on Zulip):

btw, if you compiling with --incremental right now, you are already using it

pnkfelix (Jan 24 2019 at 16:01, on Zulip):

but is --incremental synonymous with incremental=true in the config.toml? Its not, right?

mw (Jan 24 2019 at 16:01, on Zulip):

it is, as far as I know

pnkfelix (Jan 24 2019 at 16:02, on Zulip):

hmm okay I could have sworn I was observing differing behavior (in terms of one being more reliable than the other, and thus I switched my own behavior)

pnkfelix (Jan 24 2019 at 16:02, on Zulip):

anyway its past the hour and I gotta jet! Thanks for coming everyone!

mw (Jan 24 2019 at 16:02, on Zulip):

x.py's use of incremental is a bit unsound, I think

mw (Jan 24 2019 at 16:03, on Zulip):

it should really only use incr. comp. for stage 0 to be correct in all cases

mw (Jan 24 2019 at 16:03, on Zulip):

but in most cases it doesn't seem to matter

Zoxc (Jan 24 2019 at 16:04, on Zulip):

@mw Tests pass for parallel queries when we only use one thread now

mw (Jan 24 2019 at 16:05, on Zulip):

That's a good start :)

mw (Jan 24 2019 at 16:05, on Zulip):

I wonder if that would be enough to ensure correctness

mw (Jan 24 2019 at 16:05, on Zulip):

one can imagine bugs that only occur with more than one thread

mw (Jan 24 2019 at 16:06, on Zulip):

otoh the small test cases might not catch them anyway

Zoxc (Jan 24 2019 at 16:06, on Zulip):

We can't really detect race conditions in those tests anyway. We'd probably need some larger crates for that

mw (Jan 24 2019 at 16:06, on Zulip):

exactly

mw (Jan 24 2019 at 16:07, on Zulip):

the compiler compiling itself would be such a test case

mw (Jan 24 2019 at 16:07, on Zulip):

that is, we'd get at least some testing via stage 2

Zoxc (Jan 24 2019 at 17:08, on Zulip):

@mw Did you look at -Z time-passes for the hello-world regression?

Last update: Nov 16 2019 at 02:20UTC