Is there a usecase for falling back to github hosted runners for benchmarks?

in general, we try to make our usage of self-hosted runners gracefully degrade to github-hosted runners, because self-hosted runners are provided on a best-effort basis. and for bencher jobs, there are cases where we only measure binary size, so we use force-github-hosted-runner to force those cases to use github-hosted runners.

there are cases where we only measure binary size, so we use force-github-hosted-runner to force those cases to use github-hosted runners.

I already lost context here (and am a bit time constrained now, so can't look at the code again), but do non-perf related metrics like binary size need to be handled by the bencher workflow? I would imagine that the metric could be a regular output of the build workflow (and either directly uploaded, or if tokens are a problem, eventually uploaded by the bencher perf job).

In general, we try to make our usage of self-hosted runners gracefully degrade to github-hosted runners,

I think that is great, but at least for perf jobs it doesn't really make sense to me, since the perf measurements on github-hosted runners are not useful in practice due to the high volatility (counting instructions might work there, but that metric is also not incredibly useful).

I'm mainly wondering if the added complexity (to handle github-hosted runners) here is really needed or not.

I already lost context here (and am a bit time constrained now, so can't look at the code again), but do non-perf related metrics like binary size need to be handled by the bencher workflow? I would imagine that the metric could be a regular output of the build workflow (and either directly uploaded, or if tokens are a problem, eventually uploaded by the bencher perf job).

i agree. i think this would be a good rework to do in a separate patch.

I think that is great, but at least for perf jobs it doesn't really make sense to me, since the perf measurements on github-hosted runners are not useful in practice due to the high volatility (counting instructions might work there, but that metric is also not incredibly useful).

that’s fair, yea, do you think we should skip running speedometer and dromaeo if no self-hosted runners are available?

I'm mainly wondering if the added complexity (to handle github-hosted runners) here is really needed or not.

i think this is the approach with the least added complexity overall, because it takes the existing bencher job and self-hosts it using the same pattern for selecting a runner as most of our other self-hosted jobs.

without fallback (even to a github-hosted no-op job), any downtime in the self-hosted runners would break everyone’s builds, which is currently a non-starter imo. i’m working towards a world where we don’t rely on github-hosted runners and our self-hosted runners are zero maintenance, but we’re not quite there yet.

that’s fair, yea, do you think we should skip running speedometer and dromaeo if no self-hosted runners are available?

Probably we should avoid uploading metrics we aren't going to use (since there presumably is a cost, and bencher.dev is a single-person project as far as I understand). I think bencher also recently introduced rate limiting (for non paying orgs).

i think this is the approach with the least added complexity overall, because it takes the existing bencher job and self-hosts it using the same pattern for selecting a runner as most of our other self-hosted jobs.

That's fair.

without fallback (even to a github-hosted no-op job), any downtime in the self-hosted runners would break everyone’s builds, which is currently a non-starter imo.

I'm not sure I agree here. I think we do need to transition towards a model where performance benchmarking is mandatory for pull requests to be merged. Performance regressions are just as serious as test regressions, so if we can't measure, then the MQ being broken is a feature in my opinion. The main problem here in my opinion is that we don't really have redundancy in terms of infrastructure maintainers. But I guess this discussion might be better had on zulip in the TSC channel to have a bit more visibility.

If we could trigger the performance testing workflow manually, and backfill performance data after a self-hosted runner downtime, so we can easily identify regressions merged to main (and revert), that could also be a viable short-term - medium-term bandaid. I'm not sure how feasible that would be though.

I already lost context here (and am a bit time constrained now, so can't look at the code again), but do non-perf related metrics like binary size need to be handled by the bencher workflow? I would imagine that the metric could be a regular output of the build workflow (and either directly uploaded, or if tokens are a problem, eventually uploaded by the bencher perf job).

This was done because bencher requires to send all data for single point (if we would send binary size from build workflow it would show as separate point).

i was inclined to agree with you that github-hosted benchmark results are not useful, but issues like #39641 show that even our existing github-hosted benchmarking can be very useful. i do expect that self-hosted benchmark results will be more useful though.

My assumption is that we would want to prevent PRs that regress performance from getting merged eventually (but rather sooner than later). We definitely need selfhosted runners to be able to measure accurately enough to be able to add thresholds that are low enough to be generally useful. GitHub hosted runners could be used to prevent major regressions, but anything smaller than lets say 10-20% would easily fall through the cracks given the volatility we see. I agree that it is better than nothing, but we would also need to measure with github-hosted runners on every commit to establish a baseline - Since our utilization is high, and we often use 100% of our github-hosted runners, I'm not sure if this is worth it over just accepting that the merge queue would be broken if our self-hosted benchmark runners are down.

the current relationship we have with the self-hosted runners is that they’re provided on a best-effort basis, because no one is on call for outages. if you’re interested in changing that, we should discuss how to get there with the broader servo community. i don’t think this patch is the right place to unilaterally change that relationship and decide it’s acceptable for the merge queue to be broken for hours or days at a time.

I think moving to self-hosted runners is independent of this discussion, so I'd propose we move to the self-hosted runners first and then we discuss in Zulip about how strict we want to be.

BTW, in other projects like Chromium you don't get blocked due to performance regressions, the patch lands and then you get notified afterwards about the regressions, sometimes the patch has to be reverted, other times a fix can get merged and solve the regression. Just to show how other projects do this. The good part in Chromium is that the notifications are automatic opening a bug and tagging the person that landed the original patch, if we could do something like that in Bencher would be quite usefull.

i don’t think this patch is the right place to unilaterally change that relationship and decide it’s acceptable for the merge queue to be broken for hours or days at a time.

My intention was not to suggest that we do this now as part of this patch. I mainly meant to state that I believe long-term we want something like that (and yes that would discussions on how to practically implement it). I was mainly wondering if we could just not require the bencher workflows for the MQ (instead of falling back), and make it a requirement once self-hosted is stable enough, and we have relevant procedures inplace. But this PR has been cooking for long enough, so I'd be fine with merging as is and picking up the discussion another time.

BTW, in other projects like Chromium you don't get blocked due to performance regressions, the patch lands and then you get notified afterwards about the regressions, sometimes the patch has to be reverted, other times a fix can get merged and solve the regression.

I only have second-hand knowledge about this, but my understanding was that chromium CI does have multiple layers, and there are performance tests run before merging (although the number of test-configurations run only after merging is considerably larger).

The good part in Chromium is that the notifications are automatic opening a bug and tagging the person that landed the original patch, if we could do something like that in Bencher would be quite usefull.

We can setup thresholds in bencher, which would trigger a warning on bencher (as a comment in the PR if run before merging), but the usefulness of thresholds correlates strongly with the stability of the results.