str/bytes: receiver identity for whole-span cuts - #825
Conversation
WalkthroughString and bytes operations now preserve backing storage for cut results and return exact immutable receivers for unchanged operations. Formatting and conversion paths also avoid redundant string allocation. ChangesString result construction
Bytes result construction
Identity fast paths
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 041402c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/aad63bc9d3a228dd3e64914fe5a266101efe92a0/pyre-object/src/unicodeobject.rs#L238
For every whole cut, this returns the original W_UnicodeObject itself (and cut_bytes_like repeats the shortcut), whereas the cited PyPy path constructs a new wrapper whose backing string aliases the receiver and lets is_w/immutable_unique_id derive identity from that shared storage. This strengthens storage identity into wrapper-pointer identity and bypasses the representation change needed for structural parity; preserve shareable backing storage and teach the identity/UID paths about it instead of collapsing the two wrappers.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e04656629c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if piece.len() == unsafe { w_str_get_wtf8(recv) }.len() | ||
| && unsafe { is_exact_type(recv, &STR_TYPE) } | ||
| { | ||
| return recv; |
There was a problem hiding this comment.
Upstream creates a new W_UnicodeObject around the sliced RPython string and derives is_w from shared _utf8 storage; returning recv here instead hard-codes the desired identity result into selected call sites. This requires every slicing method to be manually classified and leaves operations that share storage through other paths, such as the acknowledged encode/decode case, structurally impossible to represent. Model the shared backing storage and make is_w compare it rather than returning the original wrapper; the paired cut_bytes_like shortcut needs the same treatment.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
youknowone
changed the title
`ll_stringslice_startstop` (rstr.py:867-869) returns the source string
unchanged for `start == 0 and stop >= len`, so every string method that cuts
a piece out of its receiver shares that receiver's storage when the cut spans
it whole, and `is_w` (unicodeobject.py:110-111, bytesobject.py:34-35) reports
the two identical. `descr_replace` (unicodeobject.py:1175-1176) returns
`self` on zero replacements by the same reasoning, `format_string`
(newformat.py:602-608) wraps the receiver's own storage once the spec parses
to the defaults, and `descr_str` (unicodeobject.py:333-337) returns `self`
for an exact `str`.
pyre allocated a fresh object at each of those sites, so 31 identity cases
read False where PyPy reads True: `s[:]`, `format(s, "")`, `f"{s}"`,
`f"{s!s}"`, `str.__format__(s, "")`, `s.strip()`, `s.split(',')[0]`,
`s.splitlines()[0]`, `s.replace('z', 'z')` and the bytes counterparts.
Adds `w_str_cut` / `cut_bytes_like`, which return the receiver when the
piece's byte count matches it and the receiver is an exact `str` / `bytes`.
A subclass cuts to a fresh base object and a `bytearray` always gets a fresh
mutable one, matching `is_w`'s `user_overridden_class` rejection and `_new`.
Both are for cuts only: a transform can preserve the byte count while
changing the bytes, and those have no upstream identity shortcut.
Routed through them: the str and bytes full slice, strip/lstrip/rstrip,
split/rsplit over both the separator and whitespace forms, splitlines,
partition's no-match arm, and removeprefix/removesuffix.
`wtf8_replace` and `replace_bytes` now return the `(res, replacements)` pair
`replace_count` (rstring.py:220-309) does, so replace keys its shortcut on
the count rather than on the resulting bytes -- `s.replace('a', 'a')` still
builds a new string.
`format_value_dispatch_w` is the object-returning `PyObject_Format` entry;
`format()`, FORMAT_SIMPLE/FORMAT_WITH_SPEC and `str.__format__` take it, and
`convert_value` honours `descr_str` for the `!s` conversion. `format()` also
stops returning an immortal string for a dynamic result.
`s.encode().decode() is s` stays False: PyPy shares one RPython string
between the str and the bytes, while `W_UnicodeObject.value` is a per-object
`*mut Wtf8Buf`.
Assisted-by: Claude
…rgument arm
The whole-span shortcut belongs to `ll_stringslice_startstop` alone
(rstr.py:867-869). A one-bound `s[start:]` resolves to
`ll_stringslice_startonly` (rstr.py:857-858), which calls `_ll_stringslice`
directly and always builds a fresh string, `start == 0` included.
`descr_removeprefix` (stringmethods.py:875-880, unicodeobject.py:1497-1505)
slices with that one-bound form, so an empty prefix allocates and
`b.removeprefix(b"") is b` is False; only the no-match arm rewraps the
receiver's own storage. Routing the result through `cut_bytes_like` collapsed
the two arms into one and returned the receiver for an empty prefix.
`descr_removesuffix` (stringmethods.py:882-889, unicodeobject.py:1507-1517)
guards its slice arm with `if suffix and ...`, so an empty suffix reaches the
rewrap arm and `s.removesuffix("") is s` is True. The str side matched on
`strip_suffix` alone, which succeeds for an empty suffix, and took the slice
arm.
Both now carry the upstream two-arm shape. The bytes side of removesuffix
already had the guard.
`w_str_cut` and `cut_bytes_like` document the one-bound caveat: check which
RPython slice helper the upstream arm resolves to before routing a new call
site through them.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 041402cfdd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if step == 1 | ||
| && start == 0 | ||
| && slicelength == len as i64 | ||
| && pyre_object::pyobject::is_exact_type(obj, &pyre_object::pyobject::STR_TYPE) | ||
| { |
There was a problem hiding this comment.
When the bounds are dynamic but normalize to the whole string (for example, start = 0; stop = len(s); s[start:stop]), the compiler emits BinarySlice, so this new shortcut is bypassed: runtime_ops::binary_slice_values handles str directly at lines 717-739 and still always allocates a fresh string. Consequently the common BINARY_SUBSCR path and the dedicated opcode now disagree, and the dynamic expression still reports False for is s; route the str arm of binary_slice_values through the same whole-span behavior.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
1 participant
Two parity fixes, rebased onto
origin/main(#830).1. Hand back the receiver when a cut spans it whole
ll_stringslice_startstop(rstr.py:867-869) returns the source string unchanged forstart == 0 and stop >= len, so a string method that cuts a piece out of its receiver shares that receiver's storage when the cut spans it whole, andis_w(unicodeobject.py:110-111, bytesobject.py:34-35) reports the two identical.descr_replace(unicodeobject.py:1175-1176) returnsselfon zero replacements by the same reasoning,format_string(newformat.py:602-608) wraps the receiver's own storage once the spec parses to the defaults, anddescr_str(unicodeobject.py:333-337) returnsselffor an exactstr.pyre allocated a fresh object at each of those sites, so 31 identity cases read False where PyPy reads True:
s[:],format(s, ""),f"{s}",f"{s!s}",str.__format__(s, ""),s.strip(),s.split(',')[0],s.splitlines()[0],s.replace('z', 'z')and the bytes counterparts.Adds
w_str_cut/cut_bytes_like, which return the receiver when the piece's byte count matches it and the receiver is an exactstr/bytes. A subclass cuts to a fresh base object and abytearrayalways gets a fresh mutable one, matchingis_w'suser_overridden_classrejection and_new. Cuts only: a transform can preserve the byte count while changing the bytes, and those have no upstream identity shortcut.wtf8_replaceandreplace_bytesnow return the(res, replacements)pairreplace_count(rstring.py:220-309) does, so replace keys its shortcut on the count rather than on the resulting bytes.format_value_dispatch_wis the object-returningPyObject_Formatentry;format(), FORMAT_SIMPLE/FORMAT_WITH_SPEC andstr.__format__take it, andconvert_valuehonoursdescr_strfor!s.2. Split removeprefix's slice arm from removesuffix's empty-argument arm
The whole-span shortcut belongs to
ll_stringslice_startstopalone. A one-bounds[start:]resolves toll_stringslice_startonly(rstr.py:857-858), which calls_ll_stringslicedirectly and always builds a fresh string,start == 0included.descr_removeprefix(stringmethods.py:875-880, unicodeobject.py:1497-1505) slices with that one-bound form, so an empty prefix allocates andb.removeprefix(b"") is bis False; only the no-match arm rewraps the receiver's storage. Commit 1 routed the result throughcut_bytes_like, which collapsed the two arms and returned the receiver — a regression this commit fixes before it ever reaches main.descr_removesuffix(stringmethods.py:882-889, unicodeobject.py:1507-1517) guards its slice arm withif suffix and ..., so an empty suffix reaches the rewrap arm ands.removesuffix("") is sis True. The str side matched onstrip_suffixalone and took the slice arm — a pre-existing gap, also fixed.Both helper docs now carry the one-bound caveat: check which RPython slice helper the upstream arm resolves to before routing a new call site through them.
Audited clean as two-bound: full slice, strip/lstrip/rstrip, split/rsplit (separator and whitespace), splitlines, partition's no-match arm, replace's zero-count arm.
Rebase note
Rebased onto #830. One conflict, one region, in
getitem_str: main had replaced theVec<CodePoint>materialisation with O(1) code-point indexing through_index_to_byte(unicodeobject.py:1251), while this branch added the whole-span early return. Both are orthodox and independent, so the resolution keeps both and only adapts the comparison to main'slenbecoming ausize.Known remaining divergence
s.encode().decode() is sstays False: PyPy shares one RPython string between the str and the bytes, whileW_UnicodeObject.valueis a per-object*mut Wtf8Buf. Closing it needs a representation change.Verification
Oracle is pypy3 7.3.20 (3.11.13). Negative controls are in the probes so over-application is visible, and all held: str/bytes subclasses stay
is-False and yield base types;bytearrayis False everywhere and does not alias;"%s" % s,"{}".format(s),s.lower()stay False.Run against the pre-rebase tree, with the same two commits:
removeprefix/removesuffix/strip('')/split(None,0)/replace('','')× str/bytes/bytearray/subclass): 30/30 match the oracle on both backends, was 2 mismatches before commit 2check.py331/331 on dynasm and cranelift, each at default andPYRE_FBW_MULTIFRAME=1, run seriallycargo test -p pyre-jit-trace -p pyre-jit --features dynasm: 319 passed, 0 failedcargo fmt --all --checkcleanThe same suite is being re-run against the rebased tree.
Work considered and rejected
A third commit routed the seven sites that box a code point out of a string (str slice and index,
next()on a str seq-iterator,sequence_getitem, the seq-iterator helper, GET_ITER,chr()) fromw_str_from_wtf8— which isw_str_from_wtf8_immortalbyte for byte — tow_str_from_wtf8_managed. It is not in this branch, because it fails on both axes: withPYRE_GC_INTERP=1,for ch in sover 4M code points ran 2.71s before and 7.48s after (min of three); at 40M, 74.2s before and over 120s after; peak RSS did not improve to pay for it (501 MB → 482 MB).Cause:
w_str_from_wtf8_managedcallsnote_alloc, so the safepoint runs a full old-gen mark-sweep everyCOLLECT_THRESHOLD(65536) boxed characters. That threshold is documented against "~24-40 B per int/float" (gc_interp.rs:107-109) and does not fit a workload boxing millions of one-character strings. The routing becomes worthwhile only once the safepoint throttles on allocation size rather than count.Worth recording separately: the underlying interpreter-object leak has two causes and that commit addressed only the second.
PYRE_GC_INTERPis off by default on native (gc_interp.rs:118-130), sow_str_from_wtf8_managedfalls back to the immortal path and every interpreter-allocated object is immortal regardless. The reclamation machinery itself works — RSS sampled once a second overs.lower()goes flat at ~143 MB with the flag on, and climbs past 499 MB with it off.Note for reviewers
PyPy-specific identity semantics currently have no home in the test suites —
check.pygates correctness on PyPy output, butcheck_synthetic.pybaselines on CPython, and the two disagree on several of these cases (s.removeprefix(""),s.replace("",""),s.split(None,0)[0], …). The existingremoveprefix/removesuffixtests assert equality only, never identity, which is why the regression went unnoticed.— authored by Claude