Conversation
__slots__ wjere possible__slots__ where possible
hrv-dys
left a comment
There was a problem hiding this comment.
The __slots__ additions make sense for the protocol classes — these are instantiated per-connection, so reducing per-instance memory overhead adds up under load.
A few things:
-
ServerState as a
@dataclass(slots=True)— clean conversion. Thefield(default_factory=...)for the mutable defaults (set, list) is correct and avoids the classic mutable-default-argument footgun. -
FlowControlas a dataclass — the__post_init__to callself._is_writable_event.set()is a nice touch. However, I noticerepr=False, eq=Falseon both dataclasses — is that to avoid accidentally comparing or printing these in hot paths? Might be worth a brief comment for future readers. -
The
limit_max_requestschange from@functools.cached_propertyto__init__assignment — this is necessary because__slots__andcached_propertydon't play well together (cached_property needs__dict__). The rename to_limit_max_requests()as a regular method called in__init__is a reasonable workaround. Just want to flag: this changes the laziness — it's now computed eagerly at construction time. In practice this shouldn't matter sinceServer.__init__is called once, but it's a behavioral change worth noting. -
H11Protocol and HttpToolsProtocol slots lists — I spot-checked these against the
__init__methods and they look complete. If a future PR adds an attribute and forgets to update__slots__, it'll silently fall through to a per-instance__dict__(unless I'm wrong andasyncio.Protocoldoesn't define__dict__?). Actually, since the baseasyncio.Protocoldoesn't use__slots__, subclasses still get__dict__. So the slots here act as a performance hint rather than a strict contract. That's fine, just worth being aware of.
Tests pass with only pre-existing failures (websockets deprecation warnings unrelated to this PR).
|
@hrv-dys unfortunately llm didn't catch that my changes with |
| self.write_paused = False | ||
| self._is_writable_event = asyncio.Event() | ||
| _transport: asyncio.Transport | ||
| read_paused: bool = False |
There was a problem hiding this comment.
init=False or don't use dataclasses at all
|
can be closed in favour of #2697 that already introduces slots or maybe just deffered until the mentioned one will be merged |
|
@albertedwardson I think if we can find meaningful proof of performance improvement then it's worth it, otherwise I would prefer to close it for the time being. Thanks for the push tho. |
Summary
this breaks weakrefs, but I believe the objects in question shouldn't be weakref`ed anyway
Checklist