Stream: t-compiler/wg-incr-comp

Topic: Item-relative spans


view this post on Zulip cjgillot (Apr 29 2021 at 18:00):

I recently submitted #84373. This PR changes the incr-comp encoding of spans to only encode the position relative to the surrounding definition. This PR unlocks some massive opportunities for incr compilation: up to 25% for cargo-check. More gains will be accessible later to a review of span usage by the MIR optimizer (for instance).

I am looking for a reviewer inside the work-group to evaluate the soundness of the design.

view this post on Zulip oliver (Apr 29 2021 at 18:04):

does this affect multithreaded builds?

view this post on Zulip cjgillot (Apr 29 2021 at 18:06):

Theoretically, yes.

view this post on Zulip oliver (Apr 29 2021 at 18:09):

if theoretical then no implementation

view this post on Zulip cjgillot (Apr 29 2021 at 19:52):

What do you mean?

view this post on Zulip oliver (Apr 29 2021 at 21:42):

does this pr actually involve multithreading rustc, or would that be an extension of the pr and this topic?

view this post on Zulip oliver (Apr 29 2021 at 21:42):

I think no

view this post on Zulip cjgillot (Apr 30 2021 at 17:05):

The modification is independent of multithreading, and should apply to both single and multi thread versions equally.

view this post on Zulip pnkfelix (Aug 19 2021 at 14:26):

hi @Wesley Wiser @mw and @cjgillot

view this post on Zulip pnkfelix (Aug 19 2021 at 14:26):

I figure we can talk discussion of scheduling here

view this post on Zulip mw (Aug 19 2021 at 14:27):

Aug 20, after the steering meeting works for me

view this post on Zulip Wesley Wiser (Aug 19 2021 at 14:28):

Before or after the steering meeting works for me.

view this post on Zulip pnkfelix (Aug 19 2021 at 14:34):

@cjgillot I didn’t know how to interpret your response on the other thread: did you mean that you had a conflict after the steering meeting? Or that you were available after the steering meeting?

view this post on Zulip cjgillot (Aug 19 2021 at 14:34):

I am available after the steering meeting.

view this post on Zulip pnkfelix (Aug 19 2021 at 14:38):

okay. i’ll move the event. @mw and @cjgillot , can you privmsg me the appropriate emails to send Google Calendar invites to?

view this post on Zulip pnkfelix (Aug 19 2021 at 14:38):

(or post them here if you don’t mind them being publicly archived.)

view this post on Zulip pnkfelix (Aug 20 2021 at 15:03):

Hey @Wesley Wiser @mw and @cjgillot , I forgot to say how we’d conduct the meeting

view this post on Zulip mw (Aug 20 2021 at 15:04):

we can do it right here :)

view this post on Zulip pnkfelix (Aug 20 2021 at 15:04):

sounds great to me

view this post on Zulip pnkfelix (Aug 20 2021 at 15:05):

so we’re here to discuss https://github.com/rust-lang/rust/pull/84373

view this post on Zulip pnkfelix (Aug 20 2021 at 15:06):

regarding testing: @mw , you used to have a test infrastructure that used commit histories as basis for testing incr-comp, right?

view this post on Zulip pnkfelix (Aug 20 2021 at 15:07):

I’m sitting here wondering if we could keep long-lived metadata somewhere about what commits in open-source Rust projects successfully compile. (I suppose a lot of them already have that in reports from their CI.) And then use that metadata to drive usage of commit logs

view this post on Zulip mw (Aug 20 2021 at 15:07):

yes, but that was a long time ago

view this post on Zulip pnkfelix (Aug 20 2021 at 15:08):

basically, I’m really wondering about generalizing crater to operate on histories

view this post on Zulip pnkfelix (Aug 20 2021 at 15:09):

so, you know, that slow thing can be even slower

view this post on Zulip mw (Aug 20 2021 at 15:09):

:D

view this post on Zulip mw (Aug 20 2021 at 15:09):

I think anything like that is too far off for this PR

view this post on Zulip pnkfelix (Aug 20 2021 at 15:09):

Yes that is true

view this post on Zulip pnkfelix (Aug 20 2021 at 15:09):

but its a problem that comes up again and again for us

view this post on Zulip pnkfelix (Aug 20 2021 at 15:10):

Anyway the point is, for testing, it seems like the options are either: 1. figure out a way to get more automated testing to happen for incr-comp, or 2. get people to opt into using features like this

view this post on Zulip mw (Aug 20 2021 at 15:10):

it would make for a good volunteer project: you don't have to know the compiler or Rust particularly well, just Git and so on

view this post on Zulip cjgillot (Aug 20 2021 at 15:11):

In its current form, the PR is opt-in. I still need to adapt the incr hash tests (which is tedious).

view this post on Zulip mw (Aug 20 2021 at 15:13):

I think ultimately we need both automated and manual testing

view this post on Zulip pnkfelix (Aug 20 2021 at 15:13):

@cjgillot actually makes a good point, one that @mw already said on the PR: Its behind a -Z flag. The question of “how do we validate this” is a question we can work on answering after we land the PR

view this post on Zulip pnkfelix (Aug 20 2021 at 15:13):

so we don’t need to get bogged down in that here

view this post on Zulip mw (Aug 20 2021 at 15:13):

yes, the fact that it is behind a flag makes it much easier to merge this, I think

view this post on Zulip Wesley Wiser (Aug 20 2021 at 15:14):

Apologies if this was answered in the PR thread but I didn't see it: does the PR affect performance if that flag isn't set? (I assume all of the benchmarks are with the flag enabled?)

view this post on Zulip mw (Aug 20 2021 at 15:14):

however, I'm a bit worried that the approach does not scale to a clean solution

view this post on Zulip mw (Aug 20 2021 at 15:14):

I mean, the implementation is pretty clean within the limits it has to work with

view this post on Zulip mw (Aug 20 2021 at 15:15):

but it needs to add extra hooks to dep tracking

view this post on Zulip cjgillot (Aug 20 2021 at 15:15):

Latest perf with the flag unset features ~1.8% regression for incr-unchanged.

view this post on Zulip mw (Aug 20 2021 at 15:16):

do we know where that comes from?

view this post on Zulip pnkfelix (Aug 20 2021 at 15:16):

hmm. Is there a way to guard more stuff with the flag to prevent that?

view this post on Zulip cjgillot (Aug 20 2021 at 15:16):

@mw: what would you consider as a clean solution for spans handling (in general)?

view this post on Zulip mw (Aug 20 2021 at 15:18):

one that does not require a callback into the tracking system

view this post on Zulip pnkfelix (Aug 20 2021 at 15:18):

cjgillot said:

Latest perf with the flag unset features ~1.8% regression for incr-unchanged.

is this the same as what @Vadim Petrochenkov was commenting on here ?

view this post on Zulip mw (Aug 20 2021 at 15:18):

but I don't want to block work in this area. I don't have time to work on a "clean solution" myself

view this post on Zulip cjgillot (Aug 20 2021 at 15:18):

pnkfelix said:

hmm. Is there a way to guard more stuff with the flag to prevent that?

I am not sure. There is the extra test for each span during lowering, and the modification of the span encoding.

view this post on Zulip cjgillot (Aug 20 2021 at 15:19):

pnkfelix said:

cjgillot said:

Latest perf with the flag unset features ~1.8% regression for incr-unchanged.

is this the same as what Vadim Petrochenkov was commenting on here ?

Yes.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:19):

okay

view this post on Zulip pnkfelix (Aug 20 2021 at 15:19):

You ask me, I think we should go for it

view this post on Zulip pnkfelix (Aug 20 2021 at 15:19):

as in, progress here is worth the ~1.8% hit

view this post on Zulip pnkfelix (Aug 20 2021 at 15:19):

the only other option I can imagine

view this post on Zulip pnkfelix (Aug 20 2021 at 15:19):

is to dig into what @mw is talking about

view this post on Zulip mw (Aug 20 2021 at 15:19):

@Wesley Wiser, do you know if eddyb's instruction-count based self-profiling already works?

view this post on Zulip cjgillot (Aug 20 2021 at 15:19):

I can make a PR with just the encoding change, to have a comparison baseline.

view this post on Zulip mw (Aug 20 2021 at 15:20):

that might help with pinning down the regression

view this post on Zulip mw (Aug 20 2021 at 15:20):

I'm generally OK with merging this

view this post on Zulip Wesley Wiser (Aug 20 2021 at 15:20):

I know they've used it successfully but it's not integrated into rustc yet. I think there's an open PR for that.

view this post on Zulip mw (Aug 20 2021 at 15:21):

we could enable it on nightly for check builds, maybe?

view this post on Zulip mw (Aug 20 2021 at 15:21):

I don't know how much that would help for finding bugs

view this post on Zulip pnkfelix (Aug 20 2021 at 15:21):

mw said:

we could enable it on nightly for check builds, maybe?

you mean in the CI alone?

view this post on Zulip cjgillot (Aug 20 2021 at 15:21):

We can enable it when rustc_bootstrap is set.

view this post on Zulip mw (Aug 20 2021 at 15:22):

but it would be pretty harmless if there is one, right?

view this post on Zulip mw (Aug 20 2021 at 15:22):

I meant in general

view this post on Zulip pnkfelix (Aug 20 2021 at 15:22):

cjgillot said:

We can enable it when rustc_bootstrap is set.

The motivation here is that then rustc developers would be trying it out?

view this post on Zulip mw (Aug 20 2021 at 15:22):

but yeah, just for bootstrapping might be an idea too

view this post on Zulip pnkfelix (Aug 20 2021 at 15:22):

and presumably would know how to respond if something goes wrong?

view this post on Zulip Wesley Wiser (Aug 20 2021 at 15:22):

That has not always worked well in the past :)

view this post on Zulip Wesley Wiser (Aug 20 2021 at 15:23):

But it is better than nothing

view this post on Zulip pnkfelix (Aug 20 2021 at 15:23):

hmm

view this post on Zulip mw (Aug 20 2021 at 15:23):

well, we had this problem of people assuming that incremental in bootstrap doesn't work anyway

view this post on Zulip mw (Aug 20 2021 at 15:23):

and thus we would miss bugs that were already in beta

view this post on Zulip pnkfelix (Aug 20 2021 at 15:23):

I feel like there’s potentially something here, e.g. a “surprise me” mode in config.toml that people can use to opt into being surprised.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:24):

We still have an action item to try to improve bootstrap’s handling of incremental

view this post on Zulip pnkfelix (Aug 20 2021 at 15:24):

(in terms of doing a better job of reporting to people which ICEs, i.e. from which compiler stages and building which things, represent real bugs that should be reported…)

view this post on Zulip pnkfelix (Aug 20 2021 at 15:25):

but anyway

view this post on Zulip pnkfelix (Aug 20 2021 at 15:26):

A campaign to encourage rustc devs to use incremental for bootstrap

view this post on Zulip pnkfelix (Aug 20 2021 at 15:26):

is something we should look into

view this post on Zulip pnkfelix (Aug 20 2021 at 15:27):

mw said:

one that does not require a callback into the tracking system

I want to come back to this for a moment

view this post on Zulip cjgillot (Aug 20 2021 at 15:27):

pnkfelix said:

mw said:

one that does not require a callback into the tracking system

I want to come back to this for a moment

I wholeheartedly agree

view this post on Zulip Wesley Wiser (Aug 20 2021 at 15:28):

I see there's also https://github.com/rust-lang/rust/pull/84762 which improves performance for the incr-patch scenarios. Is it likely we'd be able to land that in the next few months or is it still very much "experimental" as the title implies? I'm just trying to gauge the potential wins we're hoping for.

view this post on Zulip cjgillot (Aug 20 2021 at 15:29):

Wesley Wiser said:

I see there's also https://github.com/rust-lang/rust/pull/84762 which improves performance for the incr-patch scenarios. Is it likely we'd be able to land that in the next few months or is it still very much "experimental" as the title implies? I'm just trying to gauge the potential wins we're hoping for.

This is essentially #84373 with the flag set by default.

view this post on Zulip Wesley Wiser (Aug 20 2021 at 15:29):

Oh, I see. Thanks!

view this post on Zulip cjgillot (Aug 20 2021 at 15:30):

(at least it will be once I rebase everything properly)

view this post on Zulip mw (Aug 20 2021 at 15:31):

cjgillot said:

pnkfelix said:

mw said:

one that does not require a callback into the tracking system

I want to come back to this for a moment

I wholeheartedly agree

My general feeling is that the handling of spans already is pretty messy

view this post on Zulip mw (Aug 20 2021 at 15:32):

a lot of that comes from them being generated by the query system exists

view this post on Zulip cjgillot (Aug 20 2021 at 15:32):

My first attempt was to remove spans from the HIR. That was a disaster: large work, with little to no benefit.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:33):

mw said:

a lot of that comes from them being generated by the query system exists

spans are generated by the query system?

view this post on Zulip mw (Aug 20 2021 at 15:33):

sorry, "generated before the query system exists"

view this post on Zulip pnkfelix (Aug 20 2021 at 15:33):

oh oh oh

view this post on Zulip pnkfelix (Aug 20 2021 at 15:33):

hmm.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:34):

I assume it would be a non-starter to have the parser itself depend on the query system

view this post on Zulip mw (Aug 20 2021 at 15:34):

but maybe the PR is already as good as it can get...

view this post on Zulip cjgillot (Aug 20 2021 at 15:34):

pnkfelix said:

I assume it would be a non-starter to have the parser itself depend on the query system

This is a 2y project :)

view this post on Zulip pnkfelix (Aug 20 2021 at 15:34):

yeah okay

view this post on Zulip pnkfelix (Aug 20 2021 at 15:35):

so maybe not a non-starter, but not a short term thing

view this post on Zulip mw (Aug 20 2021 at 15:36):

cjgillot said:

My first attempt was to remove spans from the HIR. That was a disaster: large work, with little to no benefit.

It was my hope that that's the solution (IIRC)

view this post on Zulip mw (Aug 20 2021 at 15:36):

but if it doesn't work in practice ...

view this post on Zulip pnkfelix (Aug 20 2021 at 15:36):

maybe then we can talk about how that failed

view this post on Zulip pnkfelix (Aug 20 2021 at 15:37):

@cjgillot do you have a comment somewhere explaining what went wrong there?

view this post on Zulip pnkfelix (Aug 20 2021 at 15:38):

okay, it was https://github.com/rust-lang/rust/pull/72015

view this post on Zulip cjgillot (Aug 20 2021 at 15:38):

PR size : +4,244 −3,307
Approximately a rebase every week.
Regressions up to 8%, for improvements up to 12%.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:38):

so the performance hit to lowering was huge

view this post on Zulip pnkfelix (Aug 20 2021 at 15:39):

or no, you mitigated that

view this post on Zulip cjgillot (Aug 20 2021 at 15:39):

Last perf : https://github.com/rust-lang/rust/pull/72015#issuecomment-755845433

view this post on Zulip pnkfelix (Aug 20 2021 at 15:39):

comment:

For a very few cases, the query count is lower, and compilation is faster. For the rest, the query count is unchanged, and compilation is slower: it takes more work to get the spans.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:39):

its really interesting

view this post on Zulip mw (Aug 20 2021 at 15:40):

Did that also remove spans from MIR?

view this post on Zulip pnkfelix (Aug 20 2021 at 15:40):

so the item-relative span approach doesn’t have as much overhead as the approach you used in PR #72015?

view this post on Zulip cjgillot (Aug 20 2021 at 15:40):

It did not remove spans from MIR (which should have been a first step).

view this post on Zulip cjgillot (Aug 20 2021 at 15:41):

The new approach does not have a side table for spans, so most of the client code doesn't see a difference. Only diagnostics are affected.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:42):

I’d like to ask a hypothetical question, largely targeted at @mw : lets assume, for a moment, that removing spans from the HIR is the right answer, for the long term

view this post on Zulip pnkfelix (Aug 20 2021 at 15:42):

do you think that landing PR #84373 (the item-relative span encoding) would be a huge mistake, in terms of technical debt it introduces?

view this post on Zulip mw (Aug 20 2021 at 15:43):

no, I don't think so

view this post on Zulip pnkfelix (Aug 20 2021 at 15:43):

(i.e. I’m assuming here that in a year or two someone comes in and figures out a way to rip spans out of HIR anyway)

view this post on Zulip mw (Aug 20 2021 at 15:43):

what do you think about that, @cjgillot ?

view this post on Zulip cjgillot (Aug 20 2021 at 15:43):

I tend to agree.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:44):

Okay. My inclination is to push on landing PR #84373. It does provide a nice benefit (once its turned on). The architecture may not be quite as clean as we would like, but the right answer there might lie with the eventual librarification of the compiler as a whole

view this post on Zulip pnkfelix (Aug 20 2021 at 15:46):

Okay, great. So, in that case: @cjgillot , you’ll rebase (again, sorry), and I assume either @mw or @Vadim Petrochenkov will take lead on reviewing it?

view this post on Zulip pnkfelix (Aug 20 2021 at 15:46):

were there any remaining action items for the PR itself, things that needed to be addressed? I didn’t see any.

view this post on Zulip cjgillot (Aug 20 2021 at 15:46):

No problem with rebasing. I still have to adapt the incremental tests to test the option.

view this post on Zulip mw (Aug 20 2021 at 15:47):

I can do another reviewing pass but it would be great if @Vadim Petrochenkov could also take another look

view this post on Zulip pnkfelix (Aug 20 2021 at 15:47):

Okay

view this post on Zulip pnkfelix (Aug 20 2021 at 15:47):

This is great. Thanks so much for you work here @cjgillot . And thanks to @mw and @Wesley Wiser for taking the time to meet about this.

view this post on Zulip pnkfelix (Aug 20 2021 at 15:48):

(i would love for us to fire up regular wg-incr-comp meetings again, if only on zulip. I think I’ll be in a much better place to plan that in about three weeks or so, once all three of my kids are out of our house going to school.)

view this post on Zulip Joshua Nelson (Aug 20 2021 at 18:01):

pnkfelix said:

(in terms of doing a better job of reporting to people which ICEs, i.e. from which compiler stages and building which things, represent real bugs that should be reported…)

well, the issue partly that x.py has so many other bugs

view this post on Zulip Joshua Nelson (Aug 20 2021 at 18:02):

e.g. I hit https://github.com/rust-lang/rust/issues/81381 pretty regularly until I turned off llvm assertions (https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/undefined.20reference.20to.20DisableABIBreakingChecks.40LLVM_12)

view this post on Zulip Joshua Nelson (Aug 20 2021 at 18:03):

and so people get used to "broken windows"; @matklad was talking about this in the context of rust-analyzer a while ago


Last updated: Oct 21 2021 at 21:20 UTC