Stream: t-compiler

Topic: #48575 potential reversion / guard under `-Z` flag


oli (Dec 06 2018 at 10:01, on Zulip):

I'm going to partially revert the PR that removed the thread and add that thread removal under a -Z flag

pnkfelix (Dec 06 2018 at 10:01, on Zulip):

.... are you doing that with an intent to backport that change...?

oli (Dec 06 2018 at 10:03, on Zulip):

not really, we already have this stack overflow in the edition, so it's a stable -> stable regression

oli (Dec 06 2018 at 10:03, on Zulip):

it's likely nontrivial to backport, too

pnkfelix (Dec 06 2018 at 10:04, on Zulip):

just checking

pnkfelix (Dec 06 2018 at 10:05, on Zulip):

I don't object to the revert of the thread handling, but the justification is not quite clear if we don't backport, no?

Pietro Albini (Dec 06 2018 at 10:05, on Zulip):

backporting to 1.31 is out of the question, but on 1.32 why not if the patch is not too big

pnkfelix (Dec 06 2018 at 10:05, on Zulip):

(given that we are planning to separately try to address stack-usage via stacker as well? right?)

oli (Dec 06 2018 at 10:13, on Zulip):

Do we need threads for anything but the stack size? My current reading of the code seems to suggest that we only need it for stack size. So yea, reverting seems like the wrong action. Instead we should remove all the code for running a main rustc thread

nagisa (Dec 06 2018 at 11:09, on Zulip):

FWIW that main-thread related change also broke illumos

nagisa (Dec 06 2018 at 11:09, on Zulip):

(I believe it did, anyway)

nagisa (Dec 06 2018 at 11:09, on Zulip):

and I’ve been wanting to revert this for a long time already, if only to see if it would fix illumos

nagisa (Dec 06 2018 at 11:10, on Zulip):

I see no benefits whatsoever in us trying to use the main thread in anyway at all

pnkfelix (Dec 06 2018 at 11:15, on Zulip):

hold on, you two are saying "remove all code for running a main rustc thread" -- you want to do that, even to the degree of not providing a -Z flag to get the behavior desired by ishitatsuyuki on PR #48575 ?

pnkfelix (Dec 06 2018 at 11:15, on Zulip):

because I thought running on the main thread was the primary goal of that PR.

pnkfelix (Dec 06 2018 at 11:16, on Zulip):

(in order to ease the debugger UX for someone who does not want to deal with multithreaded execution)

nagisa (Dec 06 2018 at 11:17, on Zulip):

I dont mind the flag Ishi wants.

nagisa (Dec 06 2018 at 11:17, on Zulip):

but I don’t think it achieves waht you’re describing either

pnkfelix (Dec 06 2018 at 11:18, on Zulip):

okay, so the "remove all code" should be interpreted as "dont make it the default behavior" ?

nagisa (Dec 06 2018 at 11:18, on Zulip):

I did say "revert”, though that is pretty much “remove all code”…

pnkfelix (Dec 06 2018 at 11:18, on Zulip):

actually, lets move this to a targetted topic

pnkfelix (Dec 06 2018 at 11:20, on Zulip):

okay

pnkfelix (Dec 06 2018 at 11:21, on Zulip):

yeah so I guess the word "revert" can mean "revert the state of the code itself" (white-box) but it can also just mean "revert the default observable behavior" (black-box)

oli (Dec 06 2018 at 11:29, on Zulip):

Why would illumos fail due to the lack of a thread?

oli (Dec 06 2018 at 11:30, on Zulip):

That's my initial question, do we need threads at all?

oli (Dec 06 2018 at 11:31, on Zulip):

if not, once we have stacker, we can remove the leftover threading code for platforms that still need threads. The tread removal PR only removes threads on a few specific platforms and only if RUST_MIN_STACK is not specified

pnkfelix (Dec 06 2018 at 11:31, on Zulip):

ah maybe I misunderstood your point from the start here

pnkfelix (Dec 06 2018 at 11:32, on Zulip):

so you are suggesting that we keep the behavior that ishitatsuyuki desires, and just rely on stacker and thus not have to worry about RUST_MIN_STACK ?

pnkfelix (Dec 06 2018 at 11:32, on Zulip):

I think this comment from eddyb was one motivation for going back to spawning a separate thread: https://github.com/rust-lang/rust/pull/48575#issuecomment-380635967

pnkfelix (Dec 06 2018 at 11:33, on Zulip):

but I assume you've already read all the comments there, so ... have you posted counter-arguments somewhere?

oli (Dec 06 2018 at 11:33, on Zulip):

Oh oops, I forgot about that

pnkfelix (Dec 06 2018 at 11:33, on Zulip):

ah there's my answer

oli (Dec 06 2018 at 11:33, on Zulip):

I did read that, and then forgot about it over the last two days

oli (Dec 06 2018 at 11:34, on Zulip):

So I guess we're back to reverting

pnkfelix (Dec 06 2018 at 11:34, on Zulip):

right. but its still not a critical task. just something to try to do when one has time/inclination

oli (Dec 06 2018 at 11:34, on Zulip):

jup

nikomatsakis (Dec 07 2018 at 10:30, on Zulip):

And yeah, I prefer Ctrl+C plus bt to investigate performance issues, so it's a little more convenient if it's kept as a flag

@Tatsuyuki Ishi do you want to prep a PR for that? I lean against but if the PR is small and simple it also doesn't seem to do much harm

nikomatsakis (Dec 07 2018 at 10:30, on Zulip):

might be easier to have the discussion if we had a PR to examine

Tatsuyuki Ishi (Dec 07 2018 at 13:37, on Zulip):

I don't have time for the following two weeks but I will have some after. The PR should be small against a non-reverted revision (otherwise it's adding the code back again so not trivial).
What I'm feeling is that stack probe stuffs are too hard to mess with and even the current implementation cannon be called right for every platform. Do you have any other points against keeping this under the flag? It seems the convenience isn't that much when there are more trade offs.

pnkfelix (Dec 07 2018 at 13:41, on Zulip):

(for reference, here is context of niko's quote.)

Last update: Nov 22 2019 at 05:55UTC