This additionally adds support for building C extensions as part of creating a benchmark's virtual environment, since that didn't seem to be possible prior to this change.

Seefaster-cpython/ideas#370 for some background discussion.

ericsnowcurrently self-requested a review

May 16, 2022 16:25

Copy link

ContributorAuthor

mdboom commentedMay 23, 2022

Wanted to add a note: This is definitely a "microbenchmark" that isn't a great example of a real-world workload.

ericsnowcurrently requested changes

May 24, 2022

View reviewed changes

Copy link

Member

ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. There are some slight changes we should consider though, particularly usingrunner.bench_time_func() for micro benchmarks.

pyperformance/venv.py OutdatedShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/setup.pyShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.py OutdatedShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.py

Comment on lines +29 to +75

		void_foo_void()
		int_foo_int(1)
		void_foo_int(1)
		void_foo_int_int(1, 2)
		void_foo_int_int_int(1, 2, 3)
		void_foo_int_int_int_int(1, 2, 3, 4)
		void_foo_constchar(b"bytes")

Copy link

Member

Choose a reason for hiding this comment

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

The real benefit of micro-benchmarks is that it narrows down where performance regressions might be. With that in mind, would these different signatures have enough independent potential for regression that it would it make sense to have a separate benchmark for each? Would it be worth bothering even if they did?

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark_argtypes.py OutdatedShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.py OutdatedShow resolvedHide resolved

mdboom requested a review fromericsnowcurrently

May 24, 2022 21:01

mdboom force-pushed thectypes branch from0e5df59 to621ea37Compare

May 24, 2022 21:05

ericsnowcurrently requested changes

May 25, 2022

View reviewed changes

Copy link

Member

ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I just have a couple more suggestions.

pyperformance/venv.py OutdatedShow resolvedHide resolved

doc/custom_benchmarks.rst OutdatedShow resolvedHide resolved

pyperformance/_benchmark.py OutdatedShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.pyShow resolvedHide resolved

mdboom requested a review fromericsnowcurrently

May 27, 2022 17:57

kumaraditya303 self-requested a review

June 7, 2022 17:00

kumaraditya303 reviewed

Jun 8, 2022

View reviewed changes

doc/benchmarks.rst OutdatedShow resolvedHide resolved

kumaraditya303 reviewed

Jun 8, 2022

View reviewed changes

doc/benchmarks.rst OutdatedShow resolvedHide resolved

kumaraditya303 reviewed

Jun 8, 2022

View reviewed changes

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.py OutdatedShow resolvedHide resolved

kumaraditya303 reviewed

Jun 8, 2022

View reviewed changes

pyperformance/data-files/benchmarks/bm_ctypes/setup.py OutdatedShow resolvedHide resolved

mdboom force-pushed thectypes branch from7ae70c6 to55c1309Compare

June 13, 2022 12:53

mdboom requested a review fromkumaraditya303

June 13, 2022 12:54

kumaraditya303 reviewed

Jun 13, 2022

View reviewed changes

pyperformance/data-files/benchmarks/bm_ctypes/pyproject.toml OutdatedShow resolvedHide resolved

mdboom requested a review fromkumaraditya303

June 22, 2022 12:20

Copy link

Contributor

kumaraditya303 commentedJun 22, 2022

LGTM, but I am a bit concerned that benchmarking all of the call argtypes in one iteration can hide regressions, but I can't think of anyway to fix that apart from splitting this benchmark further to each call argtypes.

kumaraditya303 approved these changes

Jun 22, 2022

View reviewed changes

kumaraditya303 reviewed

Jul 5, 2022

View reviewed changes

pyperformance/data-files/benchmarks/bm_ctypes/cmodule.cShow resolvedHide resolved

ericsnowcurrently requested changes

Jul 26, 2022

View reviewed changes

Copy link

Member

ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I've left a few comments on a few minor things and to get clarification in a couple spots.

pyperformance/_benchmark.py OutdatedShow resolvedHide resolved

pyperformance/venv.py OutdatedShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.pyShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.py OutdatedShow resolvedHide resolved

pyperformance/data-files/benchmarks/bm_ctypes/run_benchmark.pyShow resolvedHide resolved

mdboom force-pushed thectypes branch from5fc3967 tofc8bd83Compare

July 26, 2022 18:21

mdboom requested a review fromericsnowcurrently

July 26, 2022 18:53

ericsnowcurrently reviewed

Jul 26, 2022

View reviewed changes

Copy link

Member

ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

A quick follow-up suggestion...

pyperformance/venv.py OutdatedShow resolvedHide resolved

pyperformance/_pip.py OutdatedShow resolvedHide resolved

ericsnowcurrently approved these changes

Jul 26, 2022

View reviewed changes

Copy link

Member

ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

I'll leave it to you about my recommended change.

mdboom requested a review fromericsnowcurrently

July 27, 2022 15:27

ericsnowcurrently approved these changes

Jul 27, 2022

View reviewed changes

Copy link

Member

ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

There's only one small suggestion, which I'll leave to your discretion.

pyperformance/_pip.py

		if os.path.isfile(req):
		name = os.path.basename(req)
		if name == "setup.py":
		req = os.path.dirname(req)

Copy link

Member

ericsnowcurrentlyJul 27, 2022

Choose a reason for hiding this comment

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

The reason we use the dirname isn't obvious, so it may be worth adding a comment here indicating pip's limitations.

pyperformance/venv.py

Comment on lines +28 to +29

		# pip doesn't support installing a setup.py,
		# but it does support installing from the directory it is in.

Copy link

Member

ericsnowcurrentlyJul 27, 2022

Choose a reason for hiding this comment

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

This comment is what I was thinking of above. Consider moving it there.

mdboom added2 commits

August 3, 2022 14:29

Support building C extensions in benchmarks

c1b7593

Add ctypes and ctypes_argtypes benchmarks

Handle setup.py in _pip.py

91a73e2

mdboom force-pushed thectypes branch fromc63fa58 to91a73e2Compare

August 3, 2022 18:30

Labels

None yet

3 participants

Movatterモバイル変換

Uh oh!

Add benchmarks for ctypes function call overhead#197

Are you sure you want to change the base?

Add benchmarks for ctypes function call overhead#197

Uh oh!

Conversation

mdboom commentedMay 10, 2022

Uh oh!

mdboom commentedMay 23, 2022

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrentlyMay 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kumaraditya303 commentedJun 22, 2022

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrentlyJul 27, 2022

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrentlyJul 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!