Stream: t-compiler

Topic: perf.rlo naming


simulacrum (May 08 2020 at 15:24, on Zulip):

Currently, perf.rust-lang.org uses the following names for the various "types" of data we collect:

I have heard from numerous people historically that these names are hard to understand (and I confuse myself all the time). I would like to propose the following, in the same order:

The intent is to think about these like "cache states"

Interested to hear what people think! cc @Nicholas Nethercote

eddyb (May 08 2020 at 15:25, on Zulip):

I have historically misunderstood "clean incremental" to mean "after running cargo clean"

eddyb (May 08 2020 at 15:26, on Zulip):

I'd say "full" instead of "empty" which is ironically the opposite

eddyb (May 08 2020 at 15:26, on Zulip):

as in, it's doing a full build

eddyb (May 08 2020 at 15:26, on Zulip):

and "incremental fresh" could be "incremental noop", since we should strive that time to be 0 :P

eddyb (May 08 2020 at 15:26, on Zulip):

or "incremental unchanged"

eddyb (May 08 2020 at 15:27, on Zulip):

cc @nikomatsakis

Jonas Schievink (May 08 2020 at 15:27, on Zulip):

They are indeed hard to understand

nikomatsakis (May 08 2020 at 15:28, on Zulip):

Hmm, empty doesn't mean much to me

nikomatsakis (May 08 2020 at 15:28, on Zulip):

I do feel "clean" means "after running clean"

nikomatsakis (May 08 2020 at 15:28, on Zulip):

as in, do a 'clean build'

nikomatsakis (May 08 2020 at 15:28, on Zulip):

(or "from scratch", I guess)

simulacrum (May 08 2020 at 15:29, on Zulip):

we can try renaming empty to "full" -- I would avoid "clean" I think because that could be seen as "working directory is clean"

nikomatsakis (May 08 2020 at 15:29, on Zulip):

I always find the use of "fresh" in cargo very confusing also, because to me a "fresh build" is a "new build", but it means fresh in the sense of "not dirty"

nikomatsakis (May 08 2020 at 15:29, on Zulip):

i.e., a fresh build in cargo, I think, means 'nothing happened', but to me it implies the opposite

nikomatsakis (May 08 2020 at 15:30, on Zulip):

I am wondering if we can make very explicit names somehow that are less confusing

nikomatsakis (May 08 2020 at 15:30, on Zulip):

(I think I like "full build")

simulacrum (May 08 2020 at 15:30, on Zulip):
nikomatsakis (May 08 2020 at 15:31, on Zulip):

That seems good. I wonder if noop might be better "no-change"?

nikomatsakis (May 08 2020 at 15:31, on Zulip):
simulacrum (May 08 2020 at 15:31, on Zulip):

I don't like noop no-change much because of the "no" prefix

nikomatsakis (May 08 2020 at 15:31, on Zulip):

that's another attempt :)

simulacrum (May 08 2020 at 15:32, on Zulip):

I think it'd be good to find some positive name for it

nikomatsakis (May 08 2020 at 15:32, on Zulip):

yeah

nikomatsakis (May 08 2020 at 15:32, on Zulip):

I think "noop" is .. probably good

nikomatsakis (May 08 2020 at 15:32, on Zulip):

it's got something of a connotation to me like .. it should be a no-op

nikomatsakis (May 08 2020 at 15:32, on Zulip):

sorry, what I mean is

nikomatsakis (May 08 2020 at 15:32, on Zulip):

it's got a connotation to me of "this command doesn't do anything"

nikomatsakis (May 08 2020 at 15:33, on Zulip):

which is of course the idea, but I mean more like "we don't even run rustc"

nikomatsakis (May 08 2020 at 15:33, on Zulip):

but anywaYI think it's good

nikomatsakis (May 08 2020 at 15:33, on Zulip):

at least I can't think of a clearer one

simulacrum (May 08 2020 at 15:33, on Zulip):

yeah maybe we should take the win

nikomatsakis (May 08 2020 at 15:33, on Zulip):

I say take the win!

nikomatsakis (May 08 2020 at 15:34, on Zulip):

we've lived with the current names long enough

simulacrum (May 08 2020 at 15:34, on Zulip):

let's try the new names out then, I'll go write up a commit or so

nikomatsakis (May 08 2020 at 15:34, on Zulip):

(and I think the idea of a "no-op build" is a useful thing to be able to talk about, and we'll get used to it)

pnkfelix (May 08 2020 at 15:35, on Zulip):

why not "unchanged" instead of "full" or "empty" ?

pnkfelix (May 08 2020 at 15:36, on Zulip):

(I guess "no-edit" is fine)

simulacrum (May 08 2020 at 15:37, on Zulip):

hm, so it's not really about the lack of changes for those

simulacrum (May 08 2020 at 15:37, on Zulip):

it's specifically "we're building without target/"

nikomatsakis (May 08 2020 at 15:40, on Zulip):

unchanged is not bad as a replacement for noop though

simulacrum (May 08 2020 at 15:54, on Zulip):

I've pushed up changes with unchanged

simulacrum (May 08 2020 at 15:55, on Zulip):

happy to hear further feedback though

Wesley Wiser (May 08 2020 at 15:55, on Zulip):

If there was perhaps a legend or tooltip or something that explained this:

clean: CARGO_INCREMENTAL=0 cargo check with no target dir
baseline incremental: CARGO_INCREMENTAL=1 cargo check with no target dir
clean incremental: CARGO_INCREMENTAL=1 cargo check with a just-populated target directory, no changes to source code
patched incremental: [patch name]: CARGO_INCREMENTAL=1 cargo check with a just-populated target directory, some patch applied to source code

I think that would be extremely helpful!

Wesley Wiser (May 08 2020 at 15:55, on Zulip):

Regardless of what we label them.

simulacrum (May 08 2020 at 15:55, on Zulip):

I've added one to the compare page

simulacrum (May 08 2020 at 15:56, on Zulip):

I'm not sure how to put it on the graph page though -- it just doesn't fit nicely

Wesley Wiser (May 08 2020 at 15:57, on Zulip):

Oh, I see it now

Wesley Wiser (May 08 2020 at 15:58, on Zulip):

Ok. I think the reason I didn't even think of that is because your comment here actually shows CARGO_INCREMENTAL=1 cargo check which tells me exactly what's going on.

Wesley Wiser (May 08 2020 at 15:58, on Zulip):

I've read the labels on the compare page before and I actually thought we were doing cargo build not cargo check

simulacrum (May 08 2020 at 15:59, on Zulip):

oh I meant that as a placeholder

Wesley Wiser (May 08 2020 at 16:00, on Zulip):

Gotcha.

simulacrum (May 08 2020 at 16:00, on Zulip):

we do cargo check for -check, build for -debug and build --release for -opt

Wesley Wiser (May 08 2020 at 16:01, on Zulip):

Oh right, duh. Sorry for the noise

Nicholas Nethercote (May 08 2020 at 21:50, on Zulip):

@simulacrum Is the underlying file format unchanged? I imagine so, given that comparisons to a month ago are still working?

Nicholas Nethercote (May 08 2020 at 21:53, on Zulip):

Judging from the recent commits, seems so

Nicholas Nethercote (May 08 2020 at 21:53, on Zulip):

Which means that the commands for local profiling, such as collector profile, still use the old names

Nicholas Nethercote (May 08 2020 at 21:54, on Zulip):

Some nits:

Nicholas Nethercote (May 08 2020 at 21:54, on Zulip):
Nicholas Nethercote (May 08 2020 at 21:57, on Zulip):
Nicholas Nethercote (May 08 2020 at 21:59, on Zulip):
simulacrum (May 08 2020 at 22:00, on Zulip):

yeah underlying format is currently unchanged but I want to change that for faster reboots and such, working on some ideas (as you can probably see by commit history there's a ton of changes recently)

simulacrum (May 08 2020 at 22:00, on Zulip):

Will fix nits

Nicholas Nethercote (May 08 2020 at 22:01, on Zulip):
Nicholas Nethercote (May 08 2020 at 22:03, on Zulip):
Nicholas Nethercote (May 08 2020 at 22:04, on Zulip):
simulacrum (May 08 2020 at 22:04, on Zulip):

hm I think I'd prefer to jettison the old names as quickly as possible

simulacrum (May 08 2020 at 22:04, on Zulip):

incr seems fine, though not sure we get too much

simulacrum (May 08 2020 at 22:05, on Zulip):

hyphens I'm "shrug" on

Nicholas Nethercote (May 08 2020 at 22:06, on Zulip):

With the old names I use quotes a lot, e.g.: 'I got speedups on "clean incremental" builds'

simulacrum (May 08 2020 at 22:06, on Zulip):

yeah, makes sense. I wouldn't mind them

Nicholas Nethercote (May 08 2020 at 22:06, on Zulip):

with a hyphen, it's a bit clearer to me what it means if you see "an incremental-full run" rather than "an incremental full run" in a sentence

Nicholas Nethercote (May 08 2020 at 22:07, on Zulip):

esp. if it's in backquotes, to give fixed-width font

Nicholas Nethercote (May 08 2020 at 22:07, on Zulip):

Anyway, just my initial thoughts

simulacrum (May 08 2020 at 22:08, on Zulip):

thanks!

simulacrum (May 08 2020 at 22:08, on Zulip):

yeah I'll switch to hyphens

Nicholas Nethercote (May 08 2020 at 22:09, on Zulip):

The RunKind enum would presumably become Full, IncrFull, IncrUnchanged, IncrPatched? The last one loses the pluralness that PatchedIncrs had.

simulacrum (May 08 2020 at 22:11, on Zulip):

hm, I guess, yes

simulacrum (May 08 2020 at 22:11, on Zulip):

I forgot about that

nikomatsakis (May 08 2020 at 22:20, on Zulip):

(I prefer fewer acronyms and contractions myself)

nikomatsakis (May 08 2020 at 22:20, on Zulip):

but +1 to dashes

simulacrum (May 08 2020 at 23:13, on Zulip):

I think incr is worth it, actually, lets us fit more onto the page and such. horizontal screen space is not that valuable but seems good, especially in e.g. github comments

simulacrum (May 08 2020 at 23:13, on Zulip):

and we can always revert if it's confusing

RalfJ (May 09 2020 at 08:39, on Zulip):

simulacrum said:

we do cargo check for -check, build for -debug and build --release for -opt

would be nice to also have that documented somewhere on the compare page -- which commands are used for what.
some time ago when I tried to reproduce perf results locally for debugging, I lost a lot of time measuring the wrong thing.

RalfJ (May 09 2020 at 08:40, on Zulip):

so I have notes like this still sitting somewhere that might or might not still be accurate:

# How to reproduce...

## ... "patched incremental" (opt) benchmarks

# prepare
rm target/release/incremental/ -rf && git reset --hard HEAD && CARGO_INCREMENTAL=1 cargo build --release
# benchmark
patch -p1 < patch && CARGO_INCREMENTAL=1 perf stat -- cargo build --release
RalfJ (May 09 2020 at 08:43, on Zulip):

re: the new names and their explanation

full (non-incremental, after cargo clean)
incr-full (after cargo clean)

this makes it sound like incremental is the default, but AFAIK that is only the case for debug builds right?

RalfJ (May 09 2020 at 08:43, on Zulip):

so to reproduce incr-full "opt" I need to cargo clean && CARGO_INCREMENTAL=1 cargo build --release

RalfJ (May 09 2020 at 08:44, on Zulip):

so given that I think the page should also state that explicitly -- seems odd to emphasize "non-incremental" but then leave it unmentioned elsewhere (particularly since "incr" is abbreviated)

RalfJ (May 09 2020 at 08:45, on Zulip):

"no change to target directory" that's an odd statement, I dont usually change that dir, I change the rust files.
I get what you mean but I find this statement not very helpful. what about "with previous build cache" or "no cargo clean" or so?

simulacrum (May 09 2020 at 11:38, on Zulip):

Sure thing, can update! :)

simulacrum (May 09 2020 at 13:16, on Zulip):

@RalfJ okay, I've updated the compare page and unified the two sets of descriptions into a single one at the top of the page

for running things locally, I'm hesitant to document commands like that -- the preference should ~always be to use the collector. I guess that's not very self-explanatory though :/

RalfJ (May 09 2020 at 13:38, on Zulip):

hm, I dont recall why I did not do that back then

RalfJ (May 09 2020 at 13:38, on Zulip):

how hard is it to run the collector against a locally built stage 1 toolchain?

RalfJ (May 09 2020 at 13:39, on Zulip):

preferrably running just the one test one is looking at

RalfJ (May 09 2020 at 13:40, on Zulip):

explanations look good. :) just wondering:

full: a non-incremental full build

no more "starting with empty cache"?

simulacrum (May 09 2020 at 16:55, on Zulip):

it should be fairly easy

simulacrum (May 09 2020 at 16:55, on Zulip):

(naming is currently a bit in flux but that'll resolve itself relatively soon)

Last update: May 29 2020 at 16:55UTC