Commit d46a72e1 authored by Gabor Greif's avatar Gabor Greif Committed by Ben Gamari

Fix comment typos

The below is only necessary to fix the CI perf fluke that
happened in 9897e8c8:
-------------------------
Metric Decrease:
    T5837
    T6048
    T9020
    T12425
    T12234
    T13035
    T12150
    Naperian
-------------------------
parent 0a4ca9eb
Pipeline #13603 failed with stages
in 596 minutes and 42 seconds
......@@ -572,7 +572,7 @@ isSimpleScrut _ _ = return False
isSimpleOp :: StgOp -> [StgArg] -> FCode Bool
-- True iff the op cannot block or allocate
isSimpleOp (StgFCallOp (CCall (CCallSpec _ _ safe)) _) _ = return $! not (playSafe safe)
-- dataToTag# evalautes its argument, see Note [dataToTag#] in primops.txt.pp
-- dataToTag# evaluates its argument, see Note [dataToTag#] in primops.txt.pp
isSimpleOp (StgPrimOp DataToTagOp) _ = return False
isSimpleOp (StgPrimOp op) stg_args = do
arg_exprs <- getNonVoidArgAmodes stg_args
......
......@@ -171,7 +171,7 @@ catchException !io handler = catch io handler
-- might catch either. If you are calling @catch@ with type
-- @IO Int -> (ArithException -> IO Int) -> IO Int@ then the handler may
-- get run with @DivideByZero@ as an argument, or an @ErrorCall \"urk\"@
-- exception may be propogated further up. If you call it again, you
-- exception may be propagated further up. If you call it again, you
-- might get a the opposite behaviour. This is ok, because 'catch' is an
-- 'IO' computation.
--
......
......@@ -477,7 +477,7 @@ lockCAF (StgRegTable *reg, StgIndStatic *caf)
// Secondly I think static thunks can't have payload: anything that they
// reference should be in SRTs
ASSERT(orig_info_tbl->layout.payload.ptrs == 0);
// Becuase the payload is empty we just push the SRT
// Because the payload is empty we just push the SRT
IF_NONMOVING_WRITE_BARRIER_ENABLED {
StgThunkInfoTable *thunk_info = itbl_to_thunk_itbl(orig_info_tbl);
if (thunk_info->i.srt) {
......
  • mentioned in merge request !2192 (closed)

    Toggle commit list
  • This is madness

  • Why do comment changes cause metric changes?

  • See !2192 (comment 245745). I'm in the process of reading the discussion in !1742 (closed), but it's mostly wandering in the dark wrt. where the regressions come from. Ultimately (!1742 (comment 241777)) we just gave up when we found that this commit "fixes" things again. See the spike in this chart.

  • Amazing. At the very least the commit message should've mentioned in BIG LETTERS in the very first line that this fixes perf issues.

    I was never satisfied with it completely but I think this is one data point that shows updating perf test results via commit messages was not a good idea. We don't do it for correctness tests, why do it for perf tests? I never understood the motivation (but I was also not involved in the initial design so maybe I'm missing something).

  • This commit doesn't fix anything, it simply works around a flaw in the perf testing infrastructure that tends to mismeasure certain commits. Nobody knows why that happens one but not the other.

  • I'll just repeat what I wrote on twitter:

    I'm not opposed to updating perf tests in commit messages. Sometimes there are increases caused by a bug fix that crucially needs to do more because of correctness. And performance should never be a reason to reject correctness.

    There should be a summary listing the increases since the last release, though, so that after ~2000 intermediate commits we can have an informed decision whether a particular increase was problematic.

    I mean, the ultimate issue here is one where we distrust our CI infrastructure enough to just accept a metric increase, simply because we have no clue what regressed and it's not possible to reproduce locally. I've been there multiple times (many times unjustified, because in fact there was a bug in my code) and find that's a terrible situation! Clearly, we have to find a way to trust CI again.

  • mentioned in issue #17658

    Toggle commit list
  • mentioned in issue #17686

    Toggle commit list
  • Sebastian Graf @sgraf812

    mentioned in merge request !2968

    ·

    mentioned in merge request !2968

    Toggle commit list
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment