Properly support set operations for case insensitive multidict views

[asvetlov]

For #965

[codspeed-hq]

CodSpeed Performance Report

Merging #1038 will degrade performances by 76.89%

_{Comparing ci-set-tests (b6f4459) with master (675c4ae)}

Summary

⚡ 10 improvements
❌ 16 (👁 16) regressions
✅ 218 untouched benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
👁	test_multidict_add_str[case-sensitive-c-extension-module]	1.8 ms	2 ms	-9.23%
👁	test_items_view_and[case-insensitive-c-extension-module]	155.3 µs	345.1 µs	-54.99%
👁	test_items_view_and[case-sensitive-c-extension-module]	148 µs	338.4 µs	-56.27%
👁	test_items_view_and[case-sensitive-pure-python-module]	1.4 ms	1.6 ms	-9.87%
⚡	test_items_view_is_disjoint[case-insensitive-c-extension-module]	2,676 µs	260.2 µs	×10
⚡	test_items_view_is_disjoint[case-sensitive-c-extension-module]	2,658.4 µs	257.9 µs	×10
👁	test_items_view_or[case-insensitive-c-extension-module]	170.2 µs	345.4 µs	-50.73%
👁	test_items_view_or[case-insensitive-pure-python-module]	532.3 µs	1,855.2 µs	-71.31%
👁	test_items_view_or[case-sensitive-c-extension-module]	163.4 µs	335.4 µs	-51.29%
👁	test_items_view_or[case-sensitive-pure-python-module]	375.5 µs	1,527.5 µs	-75.42%
⚡	test_items_view_sub[case-insensitive-pure-python-module]	1,359.5 µs	622.7 µs	×2.2
⚡	test_items_view_sub[case-sensitive-pure-python-module]	1,129.2 µs	411.4 µs	×2.7
👁	test_items_view_xor[case-insensitive-c-extension-module]	161.9 µs	452 µs	-64.18%
⚡	test_items_view_xor[case-insensitive-pure-python-module]	3 ms	1.9 ms	+53.83%
👁	test_items_view_xor[case-sensitive-c-extension-module]	155.1 µs	441.7 µs	-64.88%
⚡	test_items_view_xor[case-sensitive-pure-python-module]	2.5 ms	1.6 ms	+50.28%
⚡	test_keys_view_and[case-insensitive-c-extension-module]	101.3 µs	75.6 µs	+33.96%
⚡	test_keys_view_and[case-sensitive-c-extension-module]	93.5 µs	72.9 µs	+28.17%
⚡	test_keys_view_or[case-insensitive-c-extension-module]	117.3 µs	96.3 µs	+21.88%
👁	test_keys_view_or[case-insensitive-pure-python-module]	488.4 µs	1,724.9 µs	-71.68%
...	...	...	...	...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

[codecov]

Codecov Report

Attention: Patch coverage is 57.59804% with 173 lines in your changes missing coverage. Please review.

Project coverage is 91.15%. Comparing base (675c4ae) to head (b6f4459).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
multidict/_multidict_py.py	39.71%	2 Missing and 127 partials ⚠️
tests/test_multidict.py	77.31%	8 Missing and 36 partials ⚠️

❌ Your project status has failed because the head coverage (83.36%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
- Coverage   93.19%   91.15%   -2.04%     
==========================================
  Files          44       42       -2     
  Lines        5378     6060     +682     
  Branches      402      613     +211     
==========================================
+ Hits         5012     5524     +512     
- Misses         95      105      +10     
- Partials      271      431     +160     

Flag	Coverage Δ
CI-GHA	91.15% <57.59%> (-2.04%)	⬇️
MyPy	83.36% <57.59%> (-3.80%)	⬇️
pytest	100.00% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 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.

[Dreamsorcerer]

@asvetlov Please double check my last commit. That fixes the type errors.

[asvetlov]

pure-python code is done and 100% covered by tests.
C code will be ready soon.

[asvetlov]

C implementation for KeysView is ready.
ItemsView requires a little more work.

[asvetlov]

Hmm, the implementation is slightly incorrect, it always should prefer overlapping keys from the left operand.
md.keys() & {'key'} should select keys from multidict but {'key'} & md.keys() should select from the set.

[asvetlov]

Now Python impl and tests are good.

[Dreamsorcerer]

md.keys() & {'key'} should select keys from multidict but {'key'} & md.keys() should select from the set.

Are you sure about that? I think we discussed this before somewhere, and I suggested as a developer that if you are intersecting with 'key', it would be less expected that you get something that is not 'key'.

e.g. I can imagine code like this:

if (md.keys() & {"key"}) == {"key"}:
# Or
if ({"key"} & md.keys()) == {"key"}:

I don't think think the order should matter here, it should always produce a result from the user's set. If it produces 'Key' from the CI dict, then the above code would never work.

[Dreamsorcerer]

The above could also be safer if we had a CISet that was returned in this case and made the equality comparison safer regardless. But, that might be overkill to create a whole new set class.

[asvetlov]

md.keys() & {'key'} should select keys from multidict but {'key'} & md.keys() should select from the set.

Are you sure about that? I think we discussed this before somewhere, and I suggested as a developer that if you are intersecting with 'key', it would be less expected that you get something that is not 'key'.

e.g. I can imagine code like this:

if (md.keys() & {"key"}) == {"key"}:
# Or
if ({"key"} & md.keys()) == {"key"}:

I don't think think the order should matter here, it should always produce a result from the user's set. If it produces 'Key' from the CI dict, then the above code would never work.

if ({"key"} & md.keys()) == {"key"}: will try set.__and__(md) first, and it succeeds because set() could accept any iterable as the right argument.
In turn, if (["key"] & md.keys()) == {"key"}: first tries list.__and__(md) which returns NotImplemented. Than Python applies md.__rand__(list) which succeeds.

I really don't want to rely on this subtle logic. It is better to establish that the left argument controls that keys are used if they are present in both left and right arguments.

If multidict operators are called, they make sure that the comparison is performed on identities, it works well with case insensitivity. But the type is established by the left arg.

[asvetlov]

@Dreamsorcerer another point is __sub__ behavior.
It is clear that {'k1', 'k2'} - md.keys() should return a subset from the left argument, and md.keys() - {'k1', 'k2'} returns a subset of md keys, isn't it?
From a practical perspective, we could teach the library users that the left arg manages what overlapped keys are taken. For non-overlapped ones it depends on the key source, was it the left argument or the right.
E.g. CIMultiDict({'k1': 'v1'}) | {'k1', 123} will have exactly {istr('k1'), 123} result.

[Dreamsorcerer]

It is clear that {'k1', 'k2'} - md.keys() should return a subset from the left argument, and md.keys() - {'k1', 'k2'} returns a subset of md keys, isn't it?

Well, that's because the right side is never expected to have anything returned.

I still think the & can be pretty confusing if the keys in the user set are not included, but I'm not 100% convinced my suggestion is the correct one, so I won't argue it any further.

[asvetlov]

I had both variants implemented, and I've found that the PR's version is less controversial (but neither is 100% perfect, sure).

[asvetlov]

Regarding performance, I honestly don't care about the xor operation.
I suggest committing as-is, if somebody needs better numbers we could do it in a separate PR.
XOR is an unpleasant operation in terms of algorithmic complexity anyway. I had a variant with adding all items from the left, iterating over the right, discarding overlapping items and adding missing.
But it either requires a temporary dict/set, or has O(N**2) complexity :(

[bdraco]

XOR is going to be a rare enough operation that I don't think its a problem.

The speed up to the OR and AND operations are likely going to outweigh any loss to XOR performance for most cases.

[asvetlov]

Let's make an agreement that the performance of the items view is not very important (while it could be improved, sure).
The operations are supported because ItemsView ABC is declared to support them.
In the practical sense, I don't recall any application of this facility in aiohttp world.

[bdraco]

Let's make an agreement that the performance of the items view is not very important (while it could be improved, sure). The operations are supported because ItemsView ABC is declared to support them. In the practical sense, I don't recall any application of this facility in aiohttp world.

I looked though aiohttp and I didn't find any of these type of operations in aiohttp hot paths so I think its fine.

The other changes you made in the past few days are likely to be much more impactful in improving performance.

Let's merge this, and I can do a new release of multidict later today, update it in aiohttp and we can get a better idea of what the performance impacts are. Benchmarks in aio-libs/aiohttp#10654

If I'm wrong and we have these operations somewhere in an aiohttp hot path, we can revisit. I don't think thats the case however.

[bdraco]

Looks like there is a regression in the pure-python version #1111

[bdraco]

Looks like a minor performance regression in yarl aio-libs/yarl#1490 but not specific to this PR

[bdraco]

tests in #1112