gh-93911: `LOAD_ATTR_PROPERTY` by Fidget-Spinner · Pull Request #93912

gh-93911: `LOAD_ATTR_PROPERTY` by Fidget-Spinner · Pull Request #93912 · python/cpython

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

Conversation

Fidget-Spinner changed the title ~~GH-93911: LOAD_ATTR_PROPERTY~~ gh-93911: LOAD_ATTR_PROPERTY

Jun 16, 2022

Looks good, just a few minor nits.

Have you benchmarked this?

		@@ -0,0 +1,26 @@
		#ifndef Py_INTERNAL_DESCROBJECT_H

Choose a reason for hiding this comment

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

Maybe "pycore_property.h" as it is just the property struct.

Choose a reason for hiding this comment

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

I'm leaving it generic from now (and naming it after the file it came from) so that I can re-use it again in the future when I add more specialisations. (E.g. classmethod is also from the same file)

Choose a reason for hiding this comment

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

Do properties and classmethods really belong in the same file? Sure, they are both descriptors, but so are Python functions.
The file layout is a bit of an historical artifact.

Choose a reason for hiding this comment

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

If you don't want to change it for this PR, that's fine too.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Have you benchmarked this?

No, and I don't expect it to affect pypyerformance. None of the pyperformance benchmarks have property. It's only in the stats because pyperformance itself uses it. https://github.com/python/pyperformance/search?q=property

For real-world code, this should be a win, property is quite common.

None of the pyperformance benchmarks have property.

Sounds like we need more benchmarks.

Thanks for the reviews Mark. I'm merging this so that it unblocks your work (I'm also working on other specializations that are waiting on this).

(Running a refleak full test run locally before merging).

== Tests result: SUCCESS ==

396 tests OK.

40 tests skipped:
    test_curses test_dbm_gnu test_dbm_ndbm test_devpoll test_epoll
    test_fcntl test_fork1 test_gdb test_grp test_ioctl test_kqueue
    test_multiprocessing_fork test_multiprocessing_forkserver test_nis
    test_openpty test_ossaudiodev test_pipes test_poll test_posix
    test_pty test_pwd test_readline test_resource test_smtpnet
    test_socketserver test_spwd test_syslog test_threadsignals
    test_tix test_tk test_ttk_guionly test_urllib2net test_urllibnet
    test_wait3 test_wait4 test_winsound test_xmlrpc_net test_xxlimited
    test_xxtestfuzz test_zipfile64

Total duration: 20 min 51 sec