-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix SORT_REGULAR with new transitive comparison functions #20517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Girgias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Various comments and questions and this needs a rebase as I refactored the sorting code to remove a bunch of duplication.
|
@Girgias thank you for taking the time to provide the careful review! Looks like I was able to capture your sorting code refactor when I created this new branch. I'll push a fresh commit what I addressed in your code comments. Thanks again for the help! |
f6e9f05 to
374a660
Compare
Girgias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please only do the fix for the transitivity.
Optimizations can be decided later, but currently it just pollutes the PR and makes it harder to review and merge.
374a660 to
2ff1700
Compare
|
@Girgias yes, I clearly got a bit carried away haha. I decided to reimplement and force push a clean commit. Sorry for the mess I made of this PR. I have a bag full of optimizations we can save for a follow-up PR. One worth calling out would be to split |
ext/standard/array.c
Outdated
| /* Mirrors zend_std_compare_objects(), but recurses via php_array_compare_transitive() | ||
| * so nested properties obey SORT_REGULAR's transitive ordering. */ | ||
| static int php_array_compare_transitive_objects(zval *o1, zval *o2) /* {{{ */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I think might make more sense is to create a zend_std_compare_objects_ex() function that takes a function pointer for the prop table comparison if this is identical.
As hopefully the compiler will inline the behaviour properly in zend_std_compare_objects() so that it should be equivalent. As for quite a bit I was trying to understand what the point of this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just gave it a try, and benchmarked it. I saw a small, almost negligable, regression. I see Time-Weighted ΔMedian% increased ~0.9% (from -1.20% to -0.31%).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a different idea. I created a zend_object_compare_kind enum in zend_object_handlers.h and added zend_std_compare_objects_ex(), so the standard object comparator can flip between zend_compare() and a transitive variant (zend_compare_transitive() without going through a function-pointer callback.
To make that transitive mode reusable everywhere, I moved the SORT_REGULAR compare logic into Zend itself (zend_compare_transitive(), plus zend_compare_symbol_tables_transitive() and the enum-aware helpers).
This design showed a negligible difference (within measurement noise) in my benchmarks compared to the current implementation.
I'm happy to push another commit with this change if you'd like to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that you use the function pointer approach. Because duplicating a lot of code when only the continuation changes is not something I'm a fan of.
2ff1700 to
9026917
Compare
Add transitive comparison functions with deterministic ordering: - Numeric types and numeric strings compare numerically - Non-numeric strings sort after numeric types and numeric strings - NaN sorts after all other numeric values - Arrays recurse through transitive comparison - Objects (same class) recurse through transitive property comparison - Enums sort by object handle (stable grouping for array_unique) Fixes phpGH-20262
8b258f6 to
fcede41
Compare
Summary
Fixes #20262 by implementing transitive comparison for SORT_REGULAR. This ensures deterministic sort results when arrays contain mixed types, which previously could produce non-deterministic output due to non-transitive comparisons.
Highlights
php_array_compare_transitive(),php_array_smart_strcmp_transitive(), etc.) with deterministic ordering:php_array_key_compare_unstable_iandphp_array_data_compare_unstable_ito use the transitive comparator. All functions using SORT_REGULAR now use the transitive path.array_unique()(scalars, objects, nested arrays),sort()/ksort()numeric-string edge cases, and enum ordering stability.Performance Impact
Status: Unoptimized Implementation
This initial implementation prioritizes correctness and transitivity to fix the underlying stability issues. It is not yet optimized.
Overall, across 142 benchmarked operations, the changes result in an average ~1.5% performance improvement, with 92 operations faster and 50 slower. No regressions in standard comparison operations or sorts with other flags (e.g., SORT_NUMERIC).
However, there are known regressions in specific sort operations due to the overhead of the new dispatch logic:
ksort/krsort) on non-mixed keys: ~3–11% slower.Even in this unoptimized state, the new architecture yields significant wins in common scenarios:
rsort/arsort).Roadmap:
Once the transitive comparison logic is approved, I will submit a follow-up PR with finely tuned optimizations. These changes are expected to eliminate the current regressions and dramatically improve performance across all remaining operations.