Skip to content

feat: add __slots__ to istr for lower memory use - #1386

Open
ltsyk wants to merge 5 commits into
aio-libs:masterfrom
ltsyk:feat/istr-empty-slots
Open

feat: add __slots__ to istr for lower memory use#1386
ltsyk wants to merge 5 commits into
aio-libs:masterfrom
ltsyk:feat/istr-empty-slots

Conversation

@ltsyk

@ltsyk ltsyk commented Jul 30, 2026

Copy link
Copy Markdown

Summary

Add __slots__ for istr so case-insensitive keys do not allocate a per-instance __dict__ (issue #1360).

Fixes #1360

istr __slots__ to avoid per-key __dict__ (issue aio-libs#1360)
@ltsyk
ltsyk requested a review from asvetlov as a code owner July 30, 2026 00:46

@Vizonex Vizonex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ltsyk This looks way better than the last PR related to this one that I went through a while back. The workflows are failing however something I would recommend you try and do is run the tests locally when everything is considered as passing.

@ltsyk
ltsyk requested a review from webknjaz as a code owner July 30, 2026 07:40
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Jul 30, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 30, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 20.81%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 12 regressed benchmarks
✅ 230 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_iterate_multidict[ci-py] 547.1 µs 771 µs -29.04%
test_iterate_multidict_keys[ci-py] 556 µs 767.1 µs -27.51%
test_iterate_multidict_items[ci-py] 590.1 µs 808.5 µs -27.01%
test_keys_view_less_or_equal[ci-py] 612.3 µs 832.5 µs -26.45%
test_keys_view_less[ci-py] 629.6 µs 845.6 µs -25.54%
test_items_view_less[ci-py] 709.6 µs 947.6 µs -25.11%
test_items_view_less_or_equal[ci-py] 704.6 µs 933.6 µs -24.52%
test_multidict_popitem_str[ci-py] 1.5 ms 1.7 ms -14.21%
test_keys_view_sub[ci-py] 2.3 ms 2.7 ms -13.8%
test_keys_view_or[ci-py] 2.5 ms 2.8 ms -12.63%
test_items_view_or[ci-py] 2.7 ms 3.1 ms -10.77%
test_keys_view_is_disjoint[ci-py] 1.7 ms 1.9 ms -9.23%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ltsyk:feat/istr-empty-slots (32e125a) with master (957def7)

Open in CodSpeed

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.86%. Comparing base (957def7) to head (32e125a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1386   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files          28       28           
  Lines        3627     3647   +20     
  Branches      265      265           
=======================================
+ Hits         3622     3642   +20     
  Misses          3        3           
  Partials        2        2           
Flag Coverage Δ
CI-GHA 99.86% <100.00%> (+<0.01%) ⬆️
pytest 99.86% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aiolibsbot

Copy link
Copy Markdown
Contributor

PR Review — feat: add slots to istr for lower memory use

Good, focused change; the __slots__/__new__/__reduce__ trio is the right shape and brings the pure-Python istr in line with the C backend (which never had a __dict__), but one narrow pickle backward-compat path crashes.

Strengths:

  • Correctly drops = None and uses __new__ to init the slot, avoiding the slot-vs-classvar ValueError; the bare annotation is fine.
  • __reduce__ mirrors the C istr_reduce exactly ((type(self), (str(self),))), keeping the two backends behaviorally identical.
  • test_istr_has_no_instance_dict asserts observable behavior (hasattr(..., "__dict__")) and is parametrized across both backends.
  • CHANGES fragment present and correctly scoped to pure-Python.

Needs attention:

  • Protocol 0/1 pickles produced before this change (including the repo's own istr-py.pickle.0/1 fixtures) unpickle via copyreg._reconstructor, bypassing __new__, leaving __istr_identity__ unset; using such a key in a CIMultiDict raises AttributeError in _identity. The equality-only fixture test masks it. Guard the read with getattr(..., None) and add a pickle->CIMultiDict-key round-trip test.

🟡 Important

1. Old protocol 0/1 pickles unpickle to an istr with an uninitialized slot
multidict/_multidict_py.py:42-45

The __istr_identity__ slot is only initialized in __new__. That covers normal construction and protocol >=2 unpickling (which goes through __newobj__ -> istr.__new__), but it does not cover the protocol 0/1 unpickle path.

The committed fixtures tests/istr-py.pickle.0 and .1 were generated before this change and encode copy_reg._reconstructor(istr, unicode, 'str') (confirmed via pickletools.dis). On load that calls str.__new__(istr, 'str') directly, bypassing istr.__new__, so the slot is never set. I reproduced this:

obj = copyreg._reconstructor(istr, str, 'str')
obj.__istr_identity__   # AttributeError: 'istr' object has no attribute '__istr_identity__'

Why it matters: _CIMixin._identity (line 442) reads key.__istr_identity__ unguarded. Feed such an unpickled istr into a CIMultiDict as a key and it raises AttributeError instead of folding the key. This hits real users who unpickle istr data written by a released version at protocol 0/1, and it silently regresses the test_load_istr_from_file contract the repo maintains committed fixtures for.

Why CI stays green: test_load_istr_from_file only asserts s == obj and isinstance(...); str equality never touches the slot, so the broken instance passes.

Fix options:

  • Defensive read in _identity (localized, matches the fact it is the only reader): ret = getattr(key, "__istr_identity__", None).
  • Add a test that round-trips an istr through pickle and then uses it as a CIMultiDict key, so the load path is actually exercised rather than just compared for equality.
    def __new__(cls, value: object = "") -> Self:
        self = super().__new__(cls, value)
        self.__istr_identity__ = None
        return self

Checklist

  • Dual-backend parity (pure-Python matches C istr layout and reduce)
  • Pickle backward compatibility across all protocols — warning #1
  • New behavior covered by a test that would catch regressions — warning #1
  • Test asserts observable behavior, not source inspection
  • CHANGES fragment present and correctly categorized

Automated review by Kōan (Claude) HEAD=b44b0aa 3 min 52s

@Vizonex Vizonex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, would recommend you re-run pre-commit locally on your system and make a new commit once fully fixed but other than that this is perfect.

@ltsyk

ltsyk commented Aug 1, 2026

Copy link
Copy Markdown
Author

Thanks for reviewing. I reran the complete local test suite across both backends (1,409 passed), MyPy, and all pre-commit hooks; all pass and produced no changes, so there is no follow-up formatting commit to push. The GitHub Actions matrix is also green; the remaining pre-commit.ci status is its mergeable-check service error rather than a hook failure.

Protocol 0/1 unpickling bypasses istr.__new__; use getattr in
_identity and test that loaded istr works as a CIMultiDict key.
@ltsyk

ltsyk commented Aug 1, 2026

Copy link
Copy Markdown
Author

Thanks @Vizonex — rebased and addressed the remaining pure-Python edge case:

Protocol 0/1 unpickling of older istr fixtures bypasses istr.__new__, so __istr_identity__ may be unset. _identity now uses getattr(..., None) and falls back to computing/caching the identity. Added test_load_istr_as_cimultidict_key so loaded fixtures are actually used as CIMultiDict keys, not only equality-compared.

Pre-commit / suite re-checked on this tip.

@greptile-apps

greptile-apps Bot commented Sep 5, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

Not safe to merge until the pure-Python istr constructor preserves byte decoding compatibility.

The failure was reproduced with valid byte inputs and compared directly against builtin str and the installed C implementation, which both decode those inputs successfully.

Files Needing Attention: multidict/_multidict_py.py needs an istr.__new__ signature and forwarding behavior compatible with str(value, encoding, errors).

T-Rex T-Rex Logs

What T-Rex did

  • I authored the istr bytes constructor runtime review script to support the P1 finding.
  • I captured a runtime comparison showing a TypeError in the pure-Python istr path and the C-extension baseline during decoding.
  • I ran the istr-bytes-constructor-review.py script and observed the Python encoding TypeError with exit code 0, and I confirmed that no repository sources were modified and that only evidence artifacts were authored.
  • I recorded that a second finding-comment-proof for a posted P1 finding exists without attached artifacts.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 Pure-Python istr does not preserve str bytes decoding constructor arguments

    • Bug
      • multidict._multidict_py.istr(b"caf\xe9", "latin-1") and istr(b"a\xff", "ascii", "replace") raise TypeError. The same inputs successfully decode with builtin str and the installed C-extension multidict._multidict.istr.
    • Cause
      • istr.__new__ is declared as def __new__(cls, value: object = ""), so Python rejects the optional positional encoding and errors arguments before its super().__new__(cls, value) call can delegate to str.
    • Fix
      • Change the pure-Python constructor to accept and forward encoding and errors compatibly with str (including preserving normal validation semantics), and add pure-Python regression tests for valid bytes plus encoding and errors arguments.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "Merge branch 'master' into feat/istr-emp..." | Re-trigger Greptile

__is_istr__ = True
__istr_identity__: str | None = None

def __new__(cls, value: object = "") -> Self:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Pure-Python constructor drops encoding arguments

The narrowed __new__ signature rejects valid byte-decoding calls before it can delegate to str: istr(b"caf\xe9", "latin-1") and istr(b"a\xff", "ascii", "replace") raise TypeError. Builtin str and the C-extension istr decode the same values successfully. Accept and forward encoding and errors so callers receive the same result regardless of which multidict implementation is installed.

Context Used: CLAUDE.md (source)

Artifacts

Authored istr bytes constructor runtime review script

  • The executed review script compares builtin str, pure-Python istr, and the available C-extension istr for valid bytes decoding inputs, providing the reproducible check.

Runtime comparison showing pure-Python istr TypeError while str and C istr decode

  • Captured output of the authored comparison script shows both pure-Python TypeErrors and successful expected decoding by builtin str and C istr, verifying the incompatibility.

C-extension istr bytes decoding baseline

  • Captured direct C-backend execution successfully decodes the same Latin-1 and ASCII-replace byte inputs, establishing the compatible backend baseline.

View artifacts

T-Rex Ran code and verified through T-Rex

@asvetlov asvetlov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pure-python istr deliberately doesn't use slots for the sake of performance; it is an aware memory-speed tradeoff.
Benchmarks show this choice clearly.
I doubt that we have to change the design decision.
Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add an empty tuple as __slots__ to istr

4 participants