str, bytes: answer equality with equality rather than with an ordering - #8531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughString and bytes comparisons now evaluate equality directly. Ordering comparisons retain their existing paths. The change adds ChangesComparison evaluation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized optimization changes equality handling for strings and bytes while preserving ordering behavior; no actionable merge-blocking risk remains after normal review and checks. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
PyStr's comparison, PyBytesInner's, and the specialized CompareOpStr
instruction all answered == and != by taking Ord::cmp of the two buffers
and asking whether the result was Equal. An ordering has to read the
bytes: it memcmps the common prefix even where the lengths already settle
the question. CompareOpStr bypasses the Comparable slot, so it had also
lost the identity shortcut that slot takes, and a string compared with
itself was read end to end.
Add PyComparisonOp::eval_eq, which settles Eq and Ne from an equality test
and leaves an ordering operator to the caller, and answer through it in the
three places: slice equality checks the length first, and CompareOpStr
answers an object compared with itself the way the slot it specializes does.
n=1,000,000, per comparison:
before after
s == s (the very same object) 23.21us 0.16us
s == a string one shorter 24.63us 0.17us
b == bytes one shorter 25.28us 0.20us
ba == bytearray one shorter 27.68us 0.23us
s == an equal, distinct string 23.78us 24.50us
s < an equal string 29.87us 25.07us
Assisted-by: Claude
9c04b06 to
13c127a
Compare
|
@coderabbitai full review The incremental reviews so far only saw the diff between force-pushes and skipped the files as unchanged, so this asks for a pass over the complete change. — commented by Claude |
|
✅ Action performedFull review finished. |
Found while checking whether anything else in
strstill scales worse than CPython. These are not asymptotic -- they are two O(1) answers that were being computed in O(n).What was happening
Three places answered
==and!=by takingOrd::cmpof the two buffers and asking whether the result wasEqual:impl Comparable for PyStr(builtins/str.rs)PyBytesInner::cmp(bytes_inner.rs), which servesbytesandbytearrayInstruction::CompareOpStr(frame.rs), the specialization the interpreter installs after it sees two exactstroperandsAn ordering has to read the bytes.
[u8]: Ordmemcmps the common prefix and only then compares the lengths, so"a" * 1_000_000 == "a" * 999_999-- an answer the lengths give away -- memcmped a megabyte. AndCompareOpStrbypasses theComparableslot, so it had also lost the identity shortcut the slot takes withidentical_optimization: a string compared with itself was read end to end.CPython answers both with
_PyUnicode_Equal, which startsif (str1 == str2) return 1;and then compares kinds and lengths before any content;unicode_richcompareandbytes_richcomparespecial-casePy_EQ/Py_NEthe same way.What this does
Adds
PyComparisonOp::eval_eq, which settlesEqandNefrom an equality test and returnsNonefor an ordering operator (without evaluating the test), and answers through it in the three places. Slice equality checks the length first, andCompareOpStranswers an object compared with itself the way the slot it specializes does.Measurements
n=1,000,000, per comparison, best of 7:
s == s(the very same object)s ==a string one character shorterb ==bytes one byte shorterba ==a bytearray one byte shorters ==an equal, distinct strings <an equal stringCPython answers the first four in 0.02 µs. The remaining 0.16 µs here is the interpreter loop -- an empty lambda call measures 0.12 µs on the same machine.
Verification
Behaviour is unchanged, so the tests added to
operator_comparison.pyare a guard on the two new shortcuts rather than a regression test -- they pass on main as well. They go through a function and a loop so the operands are not constants the compiler folds and the specialized instruction is actually reached, and they cover: the same object, equal distinct objects, same-length differences, prefixes ("abc" < "abcd"), the empty string, lone surrogates, astral characters,bytes/bytearrayin both directions, and comparison against a non-string.test_str,test_bytes,test_string,test_dict,test_set,test_operator,test_compare,test_richcmp,test_sort,test_userstring,test_collections,test_json: SUCCESS. (test_bytearrayandtest_unicodefail identically on main -- neither reaches its tests on either.)cargo clippy --all-targetsandcargo fmt --checkclean.Summary by CodeRabbit
Bug Fixes
Tests
Are there others?
Every
eval_ordcall site in the tree (15 outsideslot.rs), checked for the same shape -- an ordering used to answer equality where the ordering costs more:set.rs,dict.rs,iter.rs,array.rs:1286compare lengths;frame.rs's int/float specializations,int.rs,float.rscompare scalars;cert.rscompares a bool,function.rsanOption::is_none. An ordering is O(1) on all of those, so there is nothing to save.array.rs:1255already routesEq/Nethrougheq_onlybefore it reaches the ordering path -- the same shape as this change, arrived at independently.BigInt: Ordcompares sign and limb count before digits, sointequality already short-circuits on magnitude.The three fixed here are the ones whose operands are buffers, where the ordering reads O(min(len)) bytes to answer a question the lengths settle.
eval_eqsits beside two existing helpers and is not a duplicate of either:map_eqanswers only where its predicate holds (its caller handles the other side), andeq_onlydeclares the comparisonNotImplementedfor an ordering operator -- correct for a type with no ordering, wrong forstr, which has one. The doc comment says so.