Stream: t-compiler/wg-polonius

Topic: finding perf spikes


nikomatsakis (Feb 19 2019 at 22:58, on Zulip):

So @Santiago Pastorino if you remember we were testing NLL perf on perf.rust-lang.org using some kind of NLL comparison mode before. Maybe we can touch base with @simulacrum to see how hard it would be to extend the perf runner to test vs polonius, and then run such tests locally and look for big slowdowns.

nikomatsakis (Feb 19 2019 at 22:58, on Zulip):

I imagine it's not so hard

Santiago Pastorino (Feb 19 2019 at 22:59, on Zulip):

ok

Santiago Pastorino (Feb 19 2019 at 22:59, on Zulip):

will ping @simulacrum

lqd (Feb 19 2019 at 23:05, on Zulip):

not very hard, and if it's for local testing, the NLL flags are set here

nikomatsakis (Feb 19 2019 at 23:08, on Zulip):

they monitor Zulip so i imagine they'll show up sooner or later =)

Santiago Pastorino (Feb 19 2019 at 23:21, on Zulip):

what needs to be run locally?

Santiago Pastorino (Feb 19 2019 at 23:21, on Zulip):

I mean, just passing -Zpolonius there is enough?

Santiago Pastorino (Feb 19 2019 at 23:22, on Zulip):

unsure how the perf compares things there

Santiago Pastorino (Feb 19 2019 at 23:22, on Zulip):

unsure if there's two runs one with some flags and the other without flags

lqd (Feb 19 2019 at 23:28, on Zulip):

there are many modes and dimensions, but the collector collects timings for say a specific rustc commit

lqd (Feb 19 2019 at 23:29, on Zulip):

and the comparisons are made between 2 of these

lqd (Feb 19 2019 at 23:30, on Zulip):

in your case it might be interesting to look at the "local benchmarking" or "local profiling" sections of the collector readme

lqd (Feb 19 2019 at 23:31, on Zulip):

(eg maybe manually comparing results of 2 local runs, instead of using the site, one with NLL and one with Polonius, if that's what you want to compare)

simulacrum (Feb 19 2019 at 23:36, on Zulip):

@nikomatsakis We'll want to answer a couple of questions:
- Do we want to test both vs. NLL and vs. Polonius?
- How bad/good is polonius compared to NLL? We can probably afford 10-20% but if it's much beyond that we'll want to delay enabling it for all things for a while

nikomatsakis (Feb 19 2019 at 23:36, on Zulip):

@simulacrum I am imagining that we start with local runs

nikomatsakis (Feb 19 2019 at 23:36, on Zulip):

not automatic runs

nikomatsakis (Feb 19 2019 at 23:37, on Zulip):

basically I wanted to try and answer this question:

nikomatsakis (Feb 19 2019 at 23:37, on Zulip):
nikomatsakis (Feb 19 2019 at 23:37, on Zulip):

I'm pretty sure the answer is "sometimes quite bad"

nikomatsakis (Feb 19 2019 at 23:37, on Zulip):

but I don't really know the reasons why or when

simulacrum (Feb 19 2019 at 23:38, on Zulip):

hm, okay, so that should be fairly easy I think

simulacrum (Feb 19 2019 at 23:38, on Zulip):

I would go with a manual edit for the time being -- here https://github.com/rust-lang-nursery/rustc-perf/blob/master/collector/src/bin/rustc-perf-collector/execute.rs#L231 -- as @lqd mentioned -- and just run the collector with and without that patch

simulacrum (Feb 19 2019 at 23:39, on Zulip):

We could also collect some stats with a compiler PR + bors try if we were so inclined

simulacrum (Feb 19 2019 at 23:42, on Zulip):

@nikomatsakis If it'd be helpful I can probably put a hack into perf manually to make some random (non important) commit get benchmarked where NLL is the "default" and polonius is the "nll" shown in the UI

simulacrum (Feb 19 2019 at 23:42, on Zulip):

in some sense that's the reality because 2018 means NLL is universally on

simulacrum (Feb 19 2019 at 23:43, on Zulip):

(well, for new code, anyway)

nikomatsakis (Feb 19 2019 at 23:51, on Zulip):

let's start with manual editing -- @Santiago Pastorino do you see the code that @simulacrum cited above?

Santiago Pastorino (Feb 19 2019 at 23:54, on Zulip):

will check all this tomorrow morning

Santiago Pastorino (Feb 20 2019 at 04:13, on Zulip):

@nikomatsakis saw the code yeah, as @lqd also pointed out

Santiago Pastorino (Feb 20 2019 at 04:14, on Zulip):

don't remember if -Zpolonius goes also with -Zborrowck=mir and all that

Santiago Pastorino (Feb 20 2019 at 04:14, on Zulip):

and yeah, I can test it locally and print here the results

Santiago Pastorino (Feb 20 2019 at 04:16, on Zulip):

Ok, I guess the way to go is said here https://github.com/spastorino/rust/blob/06ec1a422a5ff147c3a09af7eb80db77459e797b/src/tools/compiletest/src/runtest.rs#L1807

Santiago Pastorino (Feb 20 2019 at 04:52, on Zulip):

getting a lot of ...

Santiago Pastorino (Feb 20 2019 at 04:52, on Zulip):
 stdout=
[2019-02-20T04:48:00Z INFO  collector] 12 benchmarks left
[2019-02-20T04:48:00Z INFO  collector::execute] Running script-servo: Check + [Clean, Nll, BaseIncr, CleanIncr, PatchedIncrs]
[2019-02-20T04:48:00Z INFO  collector] failed to benchmark script-servo, recorded: expected success, got exit code: 101

stderr=error: An unknown error occurred

To learn more, run the command again with --verbose.
Santiago Pastorino (Feb 20 2019 at 04:52, on Zulip):

@simulacrum ^^^

simulacrum (Feb 20 2019 at 04:53, on Zulip):

I am hesitant but feel like it's plausible that's the fault of the compiler, not us?

simulacrum (Feb 20 2019 at 04:53, on Zulip):

(us = perf)

simulacrum (Feb 20 2019 at 04:53, on Zulip):

You might get more/better logs if you do RUST_LOG=collector=trace

Santiago Pastorino (Feb 20 2019 at 04:53, on Zulip):

:+1:

Santiago Pastorino (Feb 20 2019 at 04:55, on Zulip):
 stdout=
[2019-02-20T04:52:59Z INFO  collector] 12 benchmarks left
[2019-02-20T04:52:59Z INFO  collector::execute] Running script-servo: Check + [Clean, Nll, BaseIncr, CleanIncr, PatchedIncrs]
[2019-02-20T04:52:59Z TRACE collector::execute] running: "cp" "-R" "collector/benchmarks/script-servo/." "/tmp/.tmpIFlllh"
[2019-02-20T04:52:59Z TRACE collector::execute] running: "cargo" "pkgid" "--manifest-path" "components/script/Cargo.toml"
[2019-02-20T04:52:59Z DEBUG collector::execute] "cargo" "rustc" "--manifest-path" "components/script/Cargo.toml" "-p" "file:///tmp/.tmpIFlllh/components/script#0.0.1" "--profile" "check" "--no-default-features" "--"
[2019-02-20T04:52:59Z TRACE collector::execute] running: "bash" "-c" "find . -name \'*.rs\' | xargs touch"
[2019-02-20T04:52:59Z TRACE collector::execute] running: "cargo" "rustc" "--manifest-path" "components/script/Cargo.toml" "-p" "file:///tmp/.tmpIFlllh/components/script#0.0.1" "--profile" "check" "--no-default-features" "--"
[2019-02-20T04:53:00Z INFO  collector] failed to benchmark script-servo, recorded: expected success, got exit code: 101

stderr=error: An unknown error occurred

To learn more, run the command again with --verbose.
simulacrum (Feb 20 2019 at 04:56, on Zulip):

Yep, looks like a compiler bug. I suppose maybe standard RUST_LOG=polonius-... might work? No idea what, if anything, polonius uses for logging.

simulacrum (Feb 20 2019 at 04:57, on Zulip):

@nikomatsakis might know more.

Santiago Pastorino (Feb 20 2019 at 04:57, on Zulip):

hmm

simulacrum (Feb 20 2019 at 04:57, on Zulip):

Anyway, I need to run -- I suspect you might be able to filter it down to some smaller set of benchmarks (--help should help there for specific arguments to pass)

Santiago Pastorino (Feb 20 2019 at 04:57, on Zulip):

but I was trying to use just nightly

simulacrum (Feb 20 2019 at 04:58, on Zulip):

I think nightly is potentially similarly broken?

Santiago Pastorino (Feb 20 2019 at 04:58, on Zulip):

without polonius

Santiago Pastorino (Feb 20 2019 at 04:58, on Zulip):

just as is

simulacrum (Feb 20 2019 at 04:58, on Zulip):

Oh.

simulacrum (Feb 20 2019 at 04:58, on Zulip):

Well.

simulacrum (Feb 20 2019 at 04:58, on Zulip):

....So this is not with -Zpolonius?

Santiago Pastorino (Feb 20 2019 at 04:58, on Zulip):

it is

Santiago Pastorino (Feb 20 2019 at 04:58, on Zulip):

but did without it and ... let me check again :)

simulacrum (Feb 20 2019 at 04:59, on Zulip):

anyway, I'll try and check in tomorrow morning as well but I suspect this is a polonius bug

simulacrum (Feb 20 2019 at 04:59, on Zulip):

Perf's CI and server certainly don't see this bug.

Santiago Pastorino (Feb 20 2019 at 05:00, on Zulip):

seeing the same using nightly and rustc-perf as is

Santiago Pastorino (Feb 20 2019 at 05:01, on Zulip):

running this ...

Santiago Pastorino (Feb 20 2019 at 05:01, on Zulip):

RUST_LOG=collector=trace ./target/release/collector --output-repo output bench_local test --rustc which rustc --cargo `which cargo

Santiago Pastorino (Feb 20 2019 at 05:11, on Zulip):

ok, it seems like is an error in the command I'm running

lqd (Feb 20 2019 at 06:00, on Zulip):

(perf.rlo can’t compile script-servo right now, since the Universes PR/fixes I think, so at least ignore that one update: retracted, it does compile right now, but I swear this was not the case 2 days ago, and I think there’s a comment by Simon which might be related — in any case if it’s just this one we can ignore it for now, if they all fail then yeah that’s different :)

nikomatsakis (Feb 20 2019 at 10:50, on Zulip):

certainly plausible that polonius mode is broken, but that doesn't seem to be the problem..?

Santiago Pastorino (Feb 20 2019 at 12:10, on Zulip):

pasted image

Santiago Pastorino (Feb 20 2019 at 12:11, on Zulip):

this is current nll-check benchs on my machine

Santiago Pastorino (Feb 20 2019 at 12:11, on Zulip):

pasted image

Santiago Pastorino (Feb 20 2019 at 12:11, on Zulip):

this is polonius benchs

Santiago Pastorino (Feb 20 2019 at 12:11, on Zulip):

tuple-stress :S

Santiago Pastorino (Feb 20 2019 at 12:12, on Zulip):

@nikomatsakis I'm not sure if this is more or less what you imagined, worser or better :P

simulacrum (Feb 20 2019 at 14:11, on Zulip):

Looks actually pretty similar to NLL pre-optimization, so in some ways this is good IMO

lqd (Feb 20 2019 at 14:36, on Zulip):

and IIUC these are showing the worst case behavior: since they actually compile without errors, these benchmarks would succeed the LocationInsensitive pre-pass and not run the costly-for-now full analysis

simulacrum (Feb 20 2019 at 14:38, on Zulip):

Isn't that the best case?

Matthew Jasper (Feb 20 2019 at 14:39, on Zulip):

Looks actually pretty similar to NLL pre-optimization, so in some ways this is good IMO

Well, one of the NLL optimisations is currently disabled with Polonius because it broke it.

Matthew Jasper (Feb 20 2019 at 14:43, on Zulip):

Specifically this: https://github.com/rust-lang/rust/pull/54468/files#diff-bc11632f2a43bd654fa90b6e1e4cb94f

lqd (Feb 20 2019 at 14:45, on Zulip):

@simulacrum how so ? when the quick location-insensitive analysis is enough, running the costly one is worse :) (maybe you're talking about the additional overhead when there are indeed errors ?)

simulacrum (Feb 20 2019 at 17:45, on Zulip):

@lqd These benchmarks are illustrating the best case: they are not running the costly full analysis since they succeed, right?

lqd (Feb 20 2019 at 17:49, on Zulip):

my guess would be that they are indeed running the costly analysis (even if they don't need to, as the benchmarks build without errors), because it's the default Polonius variant (and I don't think Santiago overrode it): it's not yet packaged up in a single variant where the location-insensitive success would avoid running this DatafrogOpt full analysis (but we'll get there :)

Santiago Pastorino (Feb 20 2019 at 17:53, on Zulip):

yeah, I did not modify the thing at all :)

lqd (Feb 20 2019 at 17:53, on Zulip):

rerunning the benchmarks with the environment variable POLONIUS_ALGORITHM=LocationInsensitive (IIRC) would clear that up (and eg focus on the overhead of the inactive NLL optimisation Matthew mentioned, + fact generation, etc)

Santiago Pastorino (Feb 20 2019 at 17:54, on Zulip):

ahh right, running it ...

lqd (Feb 20 2019 at 17:56, on Zulip):

@Santiago Pastorino it'd just be an additional datapoint, the overhead of DataFrogOpt you benchmarked is very interesting/important to know as well (and thanks for doing it)

Santiago Pastorino (Feb 20 2019 at 17:56, on Zulip):

yeah, exactly

Santiago Pastorino (Feb 20 2019 at 17:56, on Zulip):

we need both things

Santiago Pastorino (Feb 20 2019 at 19:20, on Zulip):

POLONIUS_ALGORITHM=LocationInsensitive RUST_LOG=info ./target/release/collector --output-repo output3 bench_local test3 --rustc /home/santiago/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc --cargo which cargo

Santiago Pastorino (Feb 20 2019 at 19:21, on Zulip):

pasted image

Santiago Pastorino (Feb 20 2019 at 19:21, on Zulip):

hmmm, I guess that's not passing the env variable to the rustc process

simulacrum (Feb 20 2019 at 19:48, on Zulip):

@Santiago Pastorino yeah, I think we have an env_clear somewhere in the code

simulacrum (Feb 20 2019 at 19:49, on Zulip):

You'll probably want to remove that or pass through the polonius variable(s)

Santiago Pastorino (Feb 20 2019 at 19:57, on Zulip):
diff --git a/collector/src/bin/rustc-perf-collector/execute.rs b/collector/src/bin/rustc-perf-collector/execute.rs
index 1f6a0cd..28b9de9 100644
--- a/collector/src/bin/rustc-perf-collector/execute.rs
+++ b/collector/src/bin/rustc-perf-collector/execute.rs
@@ -182,6 +182,7 @@ impl<'a> CargoProcess<'a> {
             // Not all cargo invocations (e.g. `cargo clean`) need all of these
             // env vars set, but it doesn't hurt to have them.
             .env_clear()
+            .env("POLONIUS_ALGORITHM", "LocationInsensitive")
             // SHELL is needed for some benchmarks' build scripts.
             .env("SHELL", env::var_os("SHELL").unwrap_or_default())
             // PATH is needed to find things like linkers used by rustc/Cargo.
@@ -229,6 +230,7 @@ impl<'a> CargoProcess<'a> {
             if self.nll {
                 cmd.arg("-Zborrowck=mir");
                 cmd.arg("-Ztwo-phase-borrows");
+                cmd.arg("-Zpolonius");
             }
             // --wrap-rustc-with is not a valid rustc flag. But rustc-fake
             // recognizes it, strips it (and its argument) out, and uses it as an
Santiago Pastorino (Feb 20 2019 at 19:57, on Zulip):

@simulacrum trying with this ^^^

simulacrum (Feb 20 2019 at 20:01, on Zulip):

seems reasonable

Santiago Pastorino (Feb 20 2019 at 21:12, on Zulip):

pasted image

Santiago Pastorino (Feb 20 2019 at 21:12, on Zulip):

this is location insensitive if the trick worked correctly

Santiago Pastorino (Feb 20 2019 at 21:13, on Zulip):

it seems like it did

Santiago Pastorino (Feb 20 2019 at 21:14, on Zulip):

html5ever and ucd are faster

Santiago Pastorino (Feb 20 2019 at 21:15, on Zulip):

there are a lot that gave errors

lqd (Feb 20 2019 at 21:22, on Zulip):

errors huh, that's interesting

Matthew Jasper (Feb 20 2019 at 21:24, on Zulip):

Location insensitive is more location-insensitive than the current NLL and in some ways, more location insensitive than the AST borrowck.

lqd (Feb 20 2019 at 21:28, on Zulip):

that's an important thing to remember

nikomatsakis (Feb 21 2019 at 16:12, on Zulip):

@Santiago Pastorino awesome, thanks! those runs look kind of like what I expected to be honest

nikomatsakis (Feb 21 2019 at 16:13, on Zulip):

@Santiago Pastorino are you able to get memory usage statistics?

nikomatsakis (Feb 21 2019 at 16:13, on Zulip):

I have been expecting that there will be a few benchmarks (perhaps tuple-stress?) where we are building up gigantic vectors

Santiago Pastorino (Feb 21 2019 at 16:17, on Zulip):

yes

Santiago Pastorino (Feb 21 2019 at 16:17, on Zulip):

I can do that

Santiago Pastorino (Feb 21 2019 at 16:18, on Zulip):

I guess we want that for AST, NLL and Polonius NLL, right?

Santiago Pastorino (Feb 21 2019 at 16:18, on Zulip):

or do we already have AST and NLL ones somewhere?

lqd (Feb 21 2019 at 16:19, on Zulip):

weren't they recorded when you did the runs ?

lqd (Feb 21 2019 at 16:21, on Zulip):

that is, is there a combobox like this down somewhere ? if so, switching it to max-rss would give you the memory comparison

Santiago Pastorino (Feb 21 2019 at 16:23, on Zulip):

hmm, let me see

Santiago Pastorino (Feb 21 2019 at 16:23, on Zulip):

I thought that was with a different flag

Santiago Pastorino (Feb 21 2019 at 16:36, on Zulip):

pasted image

Santiago Pastorino (Feb 21 2019 at 16:37, on Zulip):

current nll

Santiago Pastorino (Feb 21 2019 at 16:37, on Zulip):

pasted image

Santiago Pastorino (Feb 21 2019 at 16:37, on Zulip):

polonius

Santiago Pastorino (Feb 21 2019 at 16:38, on Zulip):

html5ever is insanely worser

Santiago Pastorino (Feb 21 2019 at 16:38, on Zulip):

ucd

Santiago Pastorino (Feb 21 2019 at 16:38, on Zulip):

etc

Santiago Pastorino (Feb 21 2019 at 16:38, on Zulip):

and

Santiago Pastorino (Feb 21 2019 at 16:39, on Zulip):

pasted image

Santiago Pastorino (Feb 21 2019 at 16:39, on Zulip):

polonius location insensitive

Santiago Pastorino (Feb 21 2019 at 16:39, on Zulip):

talking about max-rss

Santiago Pastorino (Feb 21 2019 at 16:39, on Zulip):

just in case :)

lqd (Feb 21 2019 at 16:42, on Zulip):

:joy:

nikomatsakis (Feb 22 2019 at 11:15, on Zulip):

@Santiago Pastorino ok that was roughly what I expected =)

nikomatsakis (Feb 22 2019 at 11:16, on Zulip):

IIRC, those are places where we put in special place checks for the old NLL

nikomatsakis (Feb 22 2019 at 11:16, on Zulip):

it's probably not worth worrying about that just now, but it will be something we have to think about how to tackle at some point

Santiago Pastorino (Feb 22 2019 at 19:37, on Zulip):

:+1:

Last update: Nov 15 2019 at 20:00UTC