GeezerVison

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.

@zitterbewegung in order to make the case for landing this, it would be useful to update the PR description with some current data. Copying the description from the Cinder diff isn't really useful, since it's from a much older version. What would be helpful is to run make test with and without this change, and report any improvement in overall runtime.

It also may be that test_asyncio is no longer the longest pole, and we may need to apply the same treatment to some other modules (test_multiprocessing?) in order to see a significant benefit in overall runtime of make test.

@carljm I can see an overall benefit of 100 seconds from running cpython's baseline. I have attached both runs.
To give explicit timings before this change running make test is 5 min 26 sec and with the change it goes to 3 min and 46 seconds (100 seconds saved). Adding multiprocessing gives us a 5 second improvement.

This was executed on a AMD Ryzen Threadripper 3970X 32-Core Processor with 128GB of ram.

Patched_run_of_tests.txt
original_time_cpython.txt

@zitterbewegung Awesome, those are great results! Can you directly update the PR description/summary to eliminate the text copied from the Cinder diff and instead just summarize these results?

@zitterbewegung Awesome, those are great results! Can you directly update the PR description/summary to eliminate the text copied from the Cinder diff and instead just summarize these results?

Updated summary with new results.

@zitterbewegung Ok, one last thing I see is that make patchcheck is failing on the Azure Pipelines CI job, due to something it doesn't like about the whitespace in runtests.py. Pretty nitpicky, but we do want to keep the CI green; can you run make patchcheck locally and try to adjust the diff such that patchcheck is happy?

@zitterbewegung Awesome, those are great results! Can you directly update the PR description/summary to eliminate the text copied from the Cinder diff and instead just summarize these results?

Done.

@carljm I accidentally closed this PR while I was adding test_multiprocessing which saved 6 more seconds the new PR is #103927

@zitterbewegung Ok, one last thing I see is that make patchcheck is failing on the Azure Pipelines CI job, due to something it doesn't like about the whitespace in runtests.py. Pretty nitpicky, but we do want to keep the CI green; can you run make patchcheck locally and try to adjust the diff such that patchcheck is happy?

I ran make patch check after I did the changes