aio-libs / multidict / 36250920366

87%

push

github

Take references in update() from a dict only when lower() can run code (#1598)

<!-- Thank you for your contribution! -->

## What do these changes do?

Since #1582, `md_update_from_dict()` takes its own reference to every
key and value, because a `str` subclass key's `lower()` can run Python
code that clears the source dict. That can only happen when the target
is a `CIMultiDict` and the key is a `str` subclass other than `istr`.
Every other key goes through code that runs no Python, so the dict keeps
both alive and the borrowed references are enough. The loop now takes
references only in that one case; `extend()` still takes them, since it
moves them into the table.

The `lower()` call itself, together with the `TypeError` for a non-`str`
key, moves out of `_ci_str_to_identity()` into a `COLD` helper. The
ASCII exact-`str` path stays inline, and every `CIMultiDict` lookup gets
a little cheaper.

Instructions per operation (Callgrind,
`benchmarks/callgrind_driver.py`):

| operation | build | `MultiDict` | `CIMultiDict` |
|---|---|---:|---:|
| `update` | GIL | 39,230 -> 36,613 (-6.7%) | 45,154 -> 42,661 (-5.5%) |
| `update` | free-threaded | 49,260 -> 44,748 (-9.2%) | 55,060 -> 50,640
(-8.0%) |
| `getitem_hit` | GIL | 148 (flat) | 209 -> 201 (-3.8%) |
| `contains_hit` | GIL | 134 (flat) | 196 -> 188 (-4.1%) |
| `ctor_items` | GIL | 46,832 (flat) | 59,324 -> 57,733 (-2.7%) |

The pure-Python implementation holds references by construction and
needs no change.

## Are there changes in behavior for the user?

No, only faster `update()`, `merge()` and `CIMultiDict` key lookups.

## Is it a substantial burden for the maintainers to support this?

No. The condition is one line, next to the comment explaining it.

## Related issue number

N/A

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist (the existing
`test_key_lower_mutates_source` cases from #1582 cover the path that
still takes references)
- [x] Documentation... (continued)

781 of 1562 branches covered (50.0%)

Branch coverage included in aggregate %.

8213 of 8763 relevant lines covered (93.72%)

1.87 hits per line