Demote style-servo to only run in the "stable" set.
#1206
Conversation
nnethercote
force-pushed
the
demote-style-servo
branch
from
fbf5480 to
afd7394
Compare
|
I'm not sure I'm a fan of omitting the category getting you an implicit behavior of not running at all, seems unnecessary and rather annoying to understand/debug if you don't know about the stable-only support. I think we talked on Zulip about needing to disable the master point on the dashboard before we do this, right? Since now style-servo will no longer run there? |
I agree! It would be better if "stable" was a distinct category from "primary" and "secondary" and every benchmark belonged to exactly one category, and "supports_stable" went away. Maybe we will end up there, but there needs to be a transition period. And I didn't want to mess with the database format for now. This PR is the minimal thing that will work, I think. I chose to disable style-servo first because that will give us immediate headroom, in terms of running time, for any increases in benchmark sizes, and periods of overlap where both the old and new versions of benchmarks are live.
If it uses the |
The master point doesn't though? The dashboard just pulls from the latest master commit benchmarked and assumes the supports stable benchmarks will be there. It should be pretty easy to drop it -- just deleting these lines I think -- but I think that should probably happen here. It may also be worth a comment on that page of some kind (...this does not contain master intentionally...). |
Ah, I've now re-read and I think better understand how you've made that distinction in the code. I'm not a huge fan -- but it seems OK, and as you say, we can iterate in the future (though we should do so relatively quickly, I think). |
|
Modulo the dashboard question, I think this looks good to me. |
nnethercote
force-pushed
the
demote-style-servo
branch
from
afd7394 to
4b04f2f
Compare
(Also, temporarily, the "primary-and-stable" category. All the current "primary-and-stable" benchmarks will become "stable" in the near future.) These categories are used in `perf-config.json` and the code that chooses which benchmarks to execute. They allow the `supports_stable` field to be removed. However, when it comes to storing the category in the database, to avoid database format changes, `Category` gets split into `supports_stable` (a bool) and `category` (either "primary" or "secondary"). Some notable details: - `Category` is moved to a different file, because it's no longer used by the DB code. - Some methods are moved from `BenchmarkConfig` to `Category`. - `Category` now derives `clap::ArgEnum`, which improves the `download` command's help message by listing the possible values.
By removing its `category` field.
Because the dashboard data is collected in a totally different way, and doesn't fit nicely here.
nnethercote
force-pushed
the
demote-style-servo
branch
from
4b04f2f to
16e35f3
Compare
|
I have completely changed how I'm handling stable benchmarks, I think the new approach is much better. And I've added the master removal. The first commit is unchanged from yesterday but the latter three commits are very different and will require re-review, thanks! |
It's the longest-running benchmark in the suite, but doesn't provide results that are all that different to other benchmarks. Plus its importance in the Rust ecosystem is less than it once was. But we keep it as part of the "stable" set to preserve data continuity.
Best reviewed one commit at a time.