Add benchmarks for ctypes function call overhead by mdboom · Pull Request #197 · python/pyperformance

Add benchmarks for ctypes function call overhead by mdboom · Pull Request #197 · python/pyperformance

Conversation

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.

See faster-cpython/ideas#370 for some background discussion.

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

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 using runner.bench_time_func() for micro benchmarks.

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")

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?

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.

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.

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.

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...

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.

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.

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

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.

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.

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.

Add ctypes and ctypes_argtypes benchmarks