add condition to get commits within 30 days of the latest commit

[s7tya]

closes #1920

I'm not sure if this is a good implementation.

[Kobzol]

This is a bit hacky, yeah 😅 And it won't work for locally benchmarked commits, since these are not master commits.

Ideally, we should move the hardcoded condition that None as a left bound means "now() - 30 days" from within the Bound implementation itself, and just treat bound=none, left to be the first commit, and bound=none, right to be the last commit. But I'm a bit wary of doing that, because the bounds are used in a bunch of places, and doing this change might break a lot of stuff.

I realize now that it's quite difficult to make this work both locally and on the live server, because there we have very different requirements. On perf.RLO, we want to show only master commits, with a default range of [-30 days, now()]. Locally, we want to show all commits, with a default range of [first commit, last commit] (or [-30 days, last commit]). While the [-30 days, last commit] bound could work both for local and perf.RLO usage, the is_master() condition is a problem.

I'm not really sure what to do with that. We could display also try/local commits e.g. in debug mode, but that's quite unintuitive. We could add some parameter to the website, or check if the URL e.g. starts with localhost, but even then I wouldn't want to check this condition in Bound::[left/right]_match, but rather in comparison.rs. But for that we'd probably need to refactor the bounds quite a lot, add information to them if they should keep only master commits etc.

Maybe we could modify collector so that it puts local commits into the DB as master commits, and fills out the date for them 🤔 That could help unifying the local and production environments.

[s7tya]

I was thinking it would be useful to have a list of available benchmark runs to choose from when comparing on localhost. What do you think about adding ENV="production" and that page? As you mentioned, the desired behavior is quite different in production compared to local.

[Kobzol]

I'd ike to keep it simple-ish and the same on production and locally. What about doing this:

1. When you load the basic information from the compare page backend (it's something like /perf/info, I think), return the last N (e.g. 50) artifacts in the DB (we could just literally sort in descending order by artifact ID)
2. When you type into the artifact input box, it will suggest you/autocomplete names of the returned artifacts (with their creation date, so that you can make sense of them).

Or as an alternative to 2), add a select box. But we'd need to have both a select box and the input field, because we want to allow looking up any artifact, not just these in the last N artifacts.

[s7tya]

Option 1 looks better to me. However, to keep it simple and avoid showing alerts from the local environment, maybe we could mark the local benchmark artifact as 'master'?

[Kobzol]

I did not mean it as two options, but rather as a single solution that consists of two steps 😅 Yes, marking local artifacts as master is probably the easiest solution. In fact, I tried to just use master for the local commits and it seems to work out of the box. Why didn't I just do this before.. 😆

Could you try this? master...Kobzol:rustc-perf:bench-local-commit-type

[s7tya]

Ah, I see. I thought step 2 was a bit difficult and not the best approach.

Since it's hard to display enough information in the suggestion box due to its limited area, so we need to consider the layout carefully if we do that.

---

Thanks! I'll give it a try.

[s7tya]

Looks almost perfect! Now we don't get annoying alert.
Maybe we could also tweak this?

rustc-perf/collector/src/bin/collector.rs

Line 734 in 025f64b

let artifact_id = ArtifactId::Tag(toolchain.id.clone());

[Kobzol]

Yes, that should also be modified :) Can you update the compile/runtime artifacts to master in this PR? It's really the simplest solution. I looked at the Bound enum and doing any changes to it would require a lot of refactoring across rustc-perf.

[s7tya]

Okay. Would it be okay to close this PR and create a new one later so that the context will be easier to understand?

[Kobzol]

Ok, let's do that :)