GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add `asyncio_tcp` benchmark by kumaraditya303 · Pull Request #254 · python/pyperformance

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Dec 13, 2022

Copy link
Copy Markdown
Contributor

From python/cpython#91166

We can show off this optimization as part of 3.12 performance improvement :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@kumaraditya303
add asyncio tcp benchmark
0af52a5
@kumaraditya303
kumaraditya303 requested review from ericsnowcurrently and gvanrossum December 13, 2022 07:38
@brandtbucher

brandtbucher commented Dec 14, 2022

Copy link
Copy Markdown
Member

Is the goal for this benchmark to guide our future improvements, or just to highlight the improvement already made in the linked issue?

We should really avoid the latter... the last thing I want is to add synthetic benchmarks just to boost our 3.12 numbers.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

Is the goal for this benchmark to guide our future improvements, or just to highlight the improvement already made in the linked issue?

It is to guide for future improvements and more importantly avoid regressions. As it currently stands there are no benchmarks for asyncio thus it is hard to evaluate the impact of any bug fixes on performance, having this is pyperformance will help in tracking this. (recent python/cpython#100053)

We should really avoid the latter... the last thing I want is to add synthetic benchmarks just to boost our 3.12 numbers.

That's true though this isn't any different from other benchmark, even if there is improvement in pyperformance it doesn't mean the user's application will be that much faster or faster at all.

I am fine with keeping this in my own fork but I would prefer to have it in pyperformance so it can be tracked using speed.python.org.

ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

FYI if this is acceptable then I have other benchmarks to add for asyncio notably udp and ssl performance.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Dec 14, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Comment which does not judge this PR:
I think that adding a new benchmark will consume the cost of running time for measuring.
Since we can not add every single benchmark for each dimension, each benchmark should cover a lot of dimensions of performance if possible.

From this view,

  • asyncio-based http benchmark will be better than asyncio-based TCP benchmark.
  • asyncio-based https benchmark will be better than asyncio-based SSL benchmark.
  • asyncio-based http3 benchmark will be better than asyncio-based UDP benchmark.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

I think that adding a new benchmark will consume the cost of running time for measuring.

I was under the impression that cost of running the benchmark is not an issue. If it is an issue then it isn't worth it and I can run it myself when necessary so no need for this in pyperformance.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ericsnowcurrently

ericsnowcurrently commented Dec 14, 2022

Copy link
Copy Markdown
Member

I think that adding a new benchmark will consume the cost of running time for measuring.
Since we can not add every single benchmark for each dimension, each benchmark should cover a lot of dimensions of performance if possible.

If running time is a concern, we can restrict the set of benchmarks that are run by default. That is easily done in the manifest file. Then we can do longer runs whenever we feel comfortable with the extra time by passing the --benchmarks all (or maybe it is ""?) option to pyperformance.

In the long term, ideally we would have a solid set of workload-oriented ("macro") benchmarks that measure real-world performance (close enough, at least). This is what we would use to measure performance effectively and those results are what we would communicate to users. However, we still need the wide variety of feature-oriented ("micro") benchmarks, to help us avoid regressions and narrow down causes when we have regressions, as well as point to where possible improvements might be. See https://github.com/faster-cpython/ideas/wiki/All-About-Python-Benchmarking.

Currently, pretty much all of the pyperformance benchmarks are feature-oriented ("micro"). So we have plenty of work ahead of us. (Note that the pyston benchmarks are much closer to workload-oriented and we have been incorporating them into the faster-cpython results as much as possible.)

kumaraditya303 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Dec 15, 2022

Copy link
Copy Markdown
Member

If running time is a concern, we can restrict the set of benchmarks that are run by default.

Ah okay, in that case, I am okay to add it.
I just worried about all various of bench_asyncio_xxxx.py.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Dec 16, 2022

Copy link
Copy Markdown
Contributor Author

Final benchmark result:

Benchmark 3.10 patch
asyncio_tcp 1.41 sec 724 ms: 1.95x faster

I plan to merge this by weekend if there are no objections.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum reviewed Dec 18, 2022
View reviewed changes
Comment thread pyperformance/data-files/benchmarks/bm_asyncio_tcp/run_benchmark.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303 added 2 commits December 19, 2022 10:54
@kumaraditya303
check for received length
58a7420
@kumaraditya303
rename var
5a258f4
gvanrossum
gvanrossum approved these changes Dec 19, 2022
View reviewed changes
@gvanrossum
gvanrossum requested a review from mdboom December 19, 2022 18:27
@gvanrossum

gvanrossum commented Dec 19, 2022

Copy link
Copy Markdown
Member

@mdboom What do you think?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Dec 23, 2022

Copy link
Copy Markdown
Contributor Author

I'll merge this now as python/cpython#31871 is almost done, awaiting final review and merge. We can sort out any issues with this later down the road.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303
kumaraditya303 merged commit b3c5e70 into python:main Dec 23, 2022
pablogsal pushed a commit to pablogsal/pyperformance that referenced this pull request Jan 1, 2023
@kumaraditya303 @pablogsal
Add asyncio_tcp benchmark (python#254)
d9da8f3
Signed-off-by: Pablo Galindo <[email protected]>
pablogsal pushed a commit to pablogsal/pyperformance that referenced this pull request Jan 1, 2023
@kumaraditya303 @pablogsal
Add asyncio_tcp benchmark (python#254)
e57da59
Signed-off-by: Pablo Galindo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently
@mdboom mdboom Awaiting requested review from mdboom

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@kumaraditya303 @brandtbucher @corona10 @ericsnowcurrently @gvanrossum
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.