Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #55 +/- ##
===========================================
- Coverage 100.00% 98.68% -1.32%
===========================================
Files 3 3
Lines 437 458 +21
Branches 84 89 +5
===========================================
+ Hits 437 452 +15
- Misses 0 3 +3
- Partials 0 3 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
note that although it made it faster, it is still quadratic... here is full claude's review which points to above and also to potential regressions and with a potential fix... do not follow if do not want to see for any reason |
There was a problem hiding this comment.
isn't this line what makes it O(n) since needing copying data over again into new concatenated str?
There was a problem hiding this comment.
Copying & concatenation needs to happen no matter what; at minimum, it'll have to happen when producing each final split piece. Thus, eliminating the copy here won't change the complexity class.
There was a problem hiding this comment.
- O(n) here on each invocation could be avoided if we add into list here (O(1)) and then once (when splitting) join, making it total O(n), not O(n) on each invocation here.
- apparently there is dark magic here in Python, which optimizes so that if you do smth like
s = b.s
b.s = None
s += d
b.s = s
but not in 1 line! (since then would not work in 3.13 but would work in 3.12) , then you get overall O(n) performance. But it is not working at all on PyPy
here is the script I have
import sys
from time import monotonic
CHUNK = 65536
IMPL = sys.argv[1]
POW = int(sys.argv[2])
REPS = 3
class Buf:
__slots__ = ("s",)
def attr(b, d):
b.s += d
def local_held(b, d):
# fast local, but b.s still references the string -> refcount 2 -> copies
s = b.s
s += d
b.s = s
def local_sole(b, d):
s = b.s
b.s = None
s += d
b.s = s
def local_sole_1line(b, d):
s = b.s; b.s = None; s += d; b.s = s
def run(mb):
b = Buf()
b.s = ""
n = mb << 20
data = "x" * n
start = monotonic()
m = getattr(b, IMPL)
for i in range(0, n, CHUNK):
m(data[i: i+CHUNK])
elapsed = monotonic() - start
assert len(b.s) == n
return elapsed
print(f"# {IMPL}, best of {REPS}")
for p in range(2, POW):
mb = 2**p
print(f"{mb:3d} MiB item: {min(run(mb) for _ in range(REPS)):7.3f}s", flush=True)running which you would see surprises (disregard hardcoded cpython inside) like:
❯ for py in python3.12 python3.13 pypy3; do echo -n "====== "; $py --version; for i in attr local_sole local_sole_1line; do $py time_str_add2.py $i 6; done; done
====== Python 3.12.10
# attr, best of 3
4 MiB item: 0.008s
8 MiB item: 0.030s
16 MiB item: 0.134s
32 MiB item: 0.487s
# local_sole, best of 3
4 MiB item: 0.002s
8 MiB item: 0.002s
16 MiB item: 0.006s
32 MiB item: 0.015s
# local_sole_1line, best of 3
4 MiB item: 0.001s
8 MiB item: 0.003s
16 MiB item: 0.006s
32 MiB item: 0.016s
====== Python 3.13.5
# attr, best of 3
4 MiB item: 0.009s
8 MiB item: 0.032s
16 MiB item: 0.132s
32 MiB item: 0.485s
# local_sole, best of 3
4 MiB item: 0.001s
8 MiB item: 0.003s
16 MiB item: 0.005s
32 MiB item: 0.014s
# local_sole_1line, best of 3
4 MiB item: 0.009s
8 MiB item: 0.030s
16 MiB item: 0.135s
32 MiB item: 0.484s
====== Python 3.11.15 (7.3.23+dfsg-1, May 28 2026, 12:26:36)
[PyPy 7.3.23 with GCC 15.2.0]
# attr, best of 3
4 MiB item: 0.010s
8 MiB item: 0.056s
16 MiB item: 0.218s
32 MiB item: 0.962s
$py time_str_add2.py $i 6 2.58s user 2.69s system 99% cpu 5.266 total
# local_sole, best of 3
4 MiB item: 0.010s
8 MiB item: 0.079s
16 MiB item: 0.206s
32 MiB item: 0.920s
$py time_str_add2.py $i 6 2.77s user 3.60s system 99% cpu 6.378 total
# local_sole_1line, best of 3
4 MiB item: 0.010s
8 MiB item: 0.066s
16 MiB item: 0.199s
32 MiB item: 0.933s
$py time_str_add2.py $i 6 2.69s user 3.33s system 99% cpu 6.021 total
There was a problem hiding this comment.
beautified by claude version of the script with extra explanations etc
"""Usage: growth.py {attr|local_held|local_sole|local_sole_1line} [max_pow] [reps]
Feeds one large item into a buffer in 64 KiB chunks, the way Splitter.feed()
does, and times it against the item's size. The variants differ only in how
the grown string is stored back.
"""
import platform
import sys
from time import monotonic
CHUNK = 65536
IMPLS = ("attr", "local_held", "local_sole", "local_sole_1line")
class Buf:
__slots__ = ("s",)
def __init__(self):
self.s = ""
def attr(self, d):
# what feed() does today: the store is a STORE_ATTR, so CPython's
# in-place concatenation opcode is never eligible
self.s += d
def local_held(self, d):
# fast local, but self.s still references the string, so its refcount
# is 2 and unicode_modifiable() refuses to resize -> copies
s = self.s
s += d
self.s = s
def local_sole(self, d):
# fast local AND sole reference -> refcount 1 -> grown in place
s = self.s
self.s = None
s += d
self.s = s
def local_sole_1line(self, d):
# same statements, one source line: on CPython 3.13 the compiler emits
# STORE_FAST_LOAD_FAST here, which makes the BINARY_OP ineligible for
# the in-place specialisation and quietly reverts to copying
s = self.s; self.s = None; s += d; self.s = s
def run(impl, mb):
b = Buf()
n = mb << 20
data = "x" * n
feed = getattr(b, impl)
start = monotonic()
for i in range(0, n, CHUNK):
feed(data[i : i + CHUNK])
elapsed = monotonic() - start
assert len(b.s) == n
return elapsed
def main(impl, max_pow=6, reps=3):
if impl not in IMPLS:
sys.exit(__doc__)
print(f"# {impl}, {platform.python_implementation()} "
f"{platform.python_version()}, best of {reps}")
for p in range(2, max_pow + 1):
best = min(run(impl, 1 << p) for _ in range(reps))
print(f"{1 << p:4d} MiB item: {best:7.3f}s", flush=True)
if __name__ == "__main__":
main(sys.argv[1], *map(int, sys.argv[2:]))There was a problem hiding this comment.
the above is just the reflection of same what you @jwodder said in
at minimum, it'll have to happen when producing each final split piece
do you think this could be implemented within this PR?
|
@yarikoptic I have applied the string-concatenation trick you suggested, and I now get the following times: Is this fast enough for you? |
|
this looks like a linear |
| ----------------------- | ||
| - Support Python 3.14 | ||
| - Drop support for Python 3.8 and 3.9 | ||
| - Make splitter classes more efficient |
There was a problem hiding this comment.
I would have even elaborated as
| - Make splitter classes more efficient | |
| - Make splitter classes more efficient (linear instead of quadratic time while accumulating to split) |
or alike
|
@yarikoptic The performance improvement is now out in v0.5.2. |
2 participants
Closes #53.
Running the timing script for #53, I get the following times before this PR:
and the following times after this PR: