-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
bpo-23493: json: Change sort_keys in Python encoder same to C #8131
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
Conversation
Stop using key=lambda idiom. This behavior is same to C version encoder.
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.
Confirm that there is a benefit from this change.
Is it possible that sorted() compares two values and that the comparison raises a type error with this change? (Is it possible to have a dictionary with two keys with have the same order?) |
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.
LGTM since the C code calls list(dct.items()).sort() (no key argument).
items = list(dct.items())
items.sort()
and
items = sorted(dct.items())
are equivalent and sorted() is shorter, so LGTM :-)
It happens with C encoder.
|
Some numbers with Before patch :
After patch :
Little confused that the original issue proposed Thanks |
https://tools.ietf.org/html/rfc7159#section-4 |
@tirkarthi You compared |
Honestly, if you use such weird dictionary, you are going to get worse issues than TypeError on sorting, no? Ignore my question, your PR is fine. |
Thanks @methane missed it. Makes sense now
|
Noooo, please! If you spend hours to compile Python with PGO, don't use the timeit module which is not reliable! Use perf timeit: Your results have a large deviation:
perf timeit should help to reduce the deviation. My doc to get reliable benchmarks: |
Thanks @vstinner . The after patch doesn't make sense and I misread the posted benchmark that it's related to sorted function instead of related to JSON module and was comparing sorted function. Sorry for the noise. |
Even with perf, performance of
It seems sorting is not bottleneck. We can close the issue without merging this. |
I suggest to merge this change anyway, to respect the PEP 399: Python and C implementation must behave the same. Currently, the Python implementation is not the default but behaves differently :-( |
Stop using key=lambda idiom. This behavior is same to
C version encoder.
https://bugs.python.org/issue23493