Skip to content

gh-108303: Move Lib/test/sortperf.py to Tools/scripts #114687

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

Merged
merged 8 commits into from
Feb 18, 2024

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 28, 2024

@corona10 what do you think about moving these case to pyperformance? Would it be benefitial?

@vstinner
Copy link
Member

Which change (who) created this script? Is it @corona10?

@sobolevn
Copy link
Member Author

It was Guido 27 years ago: https://github.com/python/cpython/commits/main/Lib/test/sortperf.py

I think he agreed on moving it away in Discord's #python-dev channel.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the First thing we have to do is add the benchmark script, not remove the file.

@bedevere-app
Copy link

bedevere-app bot commented Jan 29, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@sobolevn
Copy link
Member Author

@corona10 ok, I will send a PR to pyperformance soon! 👍

@sobolevn sobolevn changed the title gh-108303: Remove Lib/test/sortperf.py gh-108303: Move Lib/test/sortperf.py to Tools/scripts Feb 9, 2024
@sobolevn sobolevn requested a review from corona10 February 9, 2024 11:57
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we just move the script into Tools/script
IMO pyperf based script is more modernized way and easy to compare with improvement patch. WDYT?

@sobolevn
Copy link
Member Author

sobolevn commented Feb 9, 2024

I agree with Guido: it is not really that useful as a benchmark (since it is a micro-benchmark). It is only interesting when working on list.sort directly. And I don't think that this is going to happen anytime soon, since the sorting algorithm is quite old, fast, and stable.

So, I think that we can move this file for now. If someone has a real use-case for list.sort benchmark in pyperf, they can recreate my existing closed PR.

And for now this file would serve more like a history reference.

@corona10
Copy link
Member

corona10 commented Feb 10, 2024

So, I think that we can move this file for now. If someone has a real use-case for list.sort benchmark in pyperf, they can recreate my existing closed PR.

Ah, I meant rewriting the benchmark script based on pyperf(you already did) and storing it Tools/script/, I did not mean to move new script to pyperformance, I also agree with Guido, it will not be a proper place.

@sobolevn
Copy link
Member Author

@corona10 sure!

@corona10
Copy link
Member

I will take a look at the PR by tomorrow :)

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. By default, all benchmark should be executed.
  2. Benchmarks needs some tune up
.....................
list_sort: Mean +- std dev: 30.8 ms +- 0.4 ms
.....................
list_sort_ascending: Mean +- std dev: 1.76 ms +- 0.10 ms

Execution time is too long :(
(Maybe we need to control inner_loops value)

@sobolevn
Copy link
Member Author

@corona10 I've merged your suggestions. Thank you!

I've also optimized the benchmark:

» time python Tools/scripts/sortperf.py  
.....................
list_sort: Mean +- std dev: 29.0 us +- 0.2 us
.....................
list_sort_ascending: Mean +- std dev: 28.9 us +- 0.4 us
.....................
list_sort_ascending_exchanged: Mean +- std dev: 29.2 us +- 0.4 us
.....................
list_sort_ascending_one_percent: Mean +- std dev: 29.5 us +- 0.2 us
.....................
list_sort_ascending_random: Mean +- std dev: 29.6 us +- 0.3 us
.....................
list_sort_descending: Mean +- std dev: 29.5 us +- 0.2 us
.....................
list_sort_duplicates: Mean +- std dev: 30.4 us +- 1.1 us
.....................
list_sort_equal: Mean +- std dev: 29.2 us +- 0.2 us
.....................
list_sort_worst_case: Mean +- std dev: 29.2 us +- 0.2 us
python Tools/scripts/sortperf.py  98.38s user 3.21s system 99% cpu 1:41.75 total

Is it good enough?

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sobolevn Feel free to merge the PR anytime!

@sobolevn
Copy link
Member Author

@corona10 Thank you! 🎉

I think that we also need to backport this to 3.11 and 3.12
Do you agree?

@corona10
Copy link
Member

Yeah go a head

@sobolevn sobolevn added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 18, 2024
@sobolevn sobolevn merged commit f9154f8 into python:main Feb 18, 2024
@miss-islington-app
Copy link

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 18, 2024
@bedevere-app
Copy link

bedevere-app bot commented Feb 18, 2024

GH-115625 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 18, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 18, 2024
@bedevere-app
Copy link

bedevere-app bot commented Feb 18, 2024

GH-115626 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 18, 2024
sobolevn added a commit that referenced this pull request Feb 18, 2024
…114687) (#115626)

gh-108303: Move `Lib/test/sortperf.py` to `Tools/scripts` (GH-114687)
(cherry picked from commit f9154f8)

Co-authored-by: Nikita Sobolev <[email protected]>
sobolevn added a commit that referenced this pull request Feb 18, 2024
…114687) (#115625)

gh-108303: Move `Lib/test/sortperf.py` to `Tools/scripts` (GH-114687)
(cherry picked from commit f9154f8)

Co-authored-by: Nikita Sobolev <[email protected]>
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants