CodSpeed Performance Report

Merging #14024 will degrade performances by 4.32%

_{Comparing david/remove-type-none (c7d4bdb) with main (e302c2d)}

Summary

❌ 1 (👁 1) regressions
✅ 31 untouched benchmarks

Benchmarks breakdown

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

I took a look at the perf regression

What I spotted is that this branch now spends 5.6% of total time inside infer_function_definition_statement where we only spent 1.6% before.

Expanding the tree shows that the difference comes from infer_decorators that isn't present in the profile for main. I suspect that it comes from

@lru_cache(maxsize=None)
def cached_tz(hour_str: str, minute_str: str, sign_str: str) -> timezone:
    sign = 1 if sign_str == "+" else -1
    return timezone(
        timedelta(
            hours=sign * int(hour_str),
            minutes=sign * int(minute_str),
        )
    )

Inside infer_decorators there's now a call to Type::none which starts semantic indexing and what not of `lru_cache.

I tried to debug if we now infer a more accurate type for lru_cache but both main and this PR infer Todo for

from functools import lru_cache
from typing import reveal_type

reveal_type(lru_cache(maxsize=None))

The main thing that's puzzling me.... Why is the lru_cache type not cached? Shouldn't this bail out early instead of calling into semantic_index?

Ohh... never mind. I looked at the warmup phase instead of the incremental path.

This is for the incremental part

The finding is the same... and, for some reason, KnownClass::none is re-executing semantic indexing of the functools module which it should not... Not sure why. We should look into this

incremental check_types this PR

incremental check_types main

Looking at the salsa code. What this suggests is that we don't reuse the same salsa ids and, because of that have cache misses...

I'm not sure what's happening and/or if this is a bug in the benchmark. I copied the tomllib and manually ran the red knot CLI in watch mode and the salsa events indicate that it uses the memoized values except for _re.py that changed.

Okay, I might still have looked at the wrong profiles. I now created a PR to use a named closure for the incremental and warmup vs to avoid this in the future.

• incremental none
• incremental main

I haven't looked at it too closely because salsa's maybe changed after profiles are a bit of a pain but I strongly suspect that the difference mainly comes from that salsa now has to validate the ingredients for the module defining None? I suspect that addressing astral-sh/ty#242 could help here as well.

the difference mainly comes from that salsa now has to validate the ingredients for the module defining None

Is this a speculation or is there evidence pointing to it? I couldn't really follow which comments here are based on looking at the wrong part of the profile :)

This could be, though. I was thinking we would already have a dependence on types module (because builtins imports it), but it's possible tomllib didn't actually resolve the type of any builtin depending on types module, so our lazy per-definition inference might have meant we previously didn't have to semantic-index types module.

If this is the cause of the regression, it's mostly just a weakness in our benchmark. Real-world codebases are likely to already rely on the types module in some way; adding a core typechecking dependency on it should not really be considered a real-world regression, if the regression just comes from "now we have to index that module."

I still think it's worth seeing whether Salsa-caching the resolution of the type of None makes a difference here. If the regression is entirely due to having all the new ingredients from the types module, then it won't.

Performance investigation

• The original version of this PR had a -4% performance regression on the "incremental" as well as the "cold" benchmark (results). The "cold" benchmark was probably just slightly below the warning/error threshold.
• Consequently, a rebase to main (after the merge of Give non-existent files a durability of at least Medium #14034 by @MichaReiser) did not help with improving performance. Both versions are still at -4% (results)
• I then tried the suggestion by @carljm and added explicit salsa caching for the lookup of NoneType in 503aa46. The CI run now passes, but the performance regression is still there with -3% for both versions (results). Not sure if the resolution of these benchmarks is really sub-1%? If so, this is a small improvement.
• I checked which modules were resolved when running red-knot on tomllib; On main, we do not load _typeshed; on this branch, we do (due to the explicit lookup of NoneType in typeshed). This alone could probably explain the increase in runtime.

This also explains why I didn't see a performance regression when comparing this branch with main on a larger codebase (black). Because we do load _typeshed even on main (for some reason) when running on black. Results:

After talking to @MichaReiser, I removed the explicit caching of the NoneType lookup again.