Skip to content
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

Jump to bottom

gh-93911: LOAD_ATTR_PROPERTY #93912

Merged
merged 14 commits into from Jun 17, 2022
Merged

gh-93911: LOAD_ATTR_PROPERTY #93912

merged 14 commits into from Jun 17, 2022
+172 −71

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Jun 16, 2022 •
edited

Linked to #93911.

@Fidget-Spinner Fidget-Spinner changed the title GH-93911: LOAD_ATTR_PROPERTY gh-93911: LOAD_ATTR_PROPERTY Jun 16, 2022
markshannon
markshannon requested changes Jun 17, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

Looks good, just a few minor nits.

Have you benchmarked this?

Include/internal/pycore_descrobject.h
@@ -0,0 +1,26 @@
#ifndef Py_INTERNAL_DESCROBJECT_H
Copy link
Member

@markshannon markshannon Jun 17, 2022

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.

Copy link
Member Author

@Fidget-Spinner Fidget-Spinner Jun 17, 2022

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)

Copy link
Member

@markshannon markshannon Jun 17, 2022

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.

Copy link
Member

@markshannon markshannon Jun 17, 2022

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.

Python/ceval.c Outdated Show resolved Hide resolved
Python/specialize.c Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 17, 2022

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

@Fidget-Spinner
Copy link
Member Author

@Fidget-Spinner Fidget-Spinner commented Jun 17, 2022

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.

@markshannon
Copy link
Member

@markshannon markshannon commented Jun 17, 2022

None of the pyperformance benchmarks have property.

Sounds like we need more benchmarks.

@markshannon markshannon mentioned this pull request Jun 17, 2022
Open
@Fidget-Spinner
Copy link
Member Author

@Fidget-Spinner Fidget-Spinner commented Jun 17, 2022 •
edited

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

@Fidget-Spinner
Copy link
Member Author

@Fidget-Spinner Fidget-Spinner commented Jun 17, 2022 

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

@Fidget-Spinner Fidget-Spinner merged commit a51742a into python:main Jun 17, 2022
14 checks passed
@Fidget-Spinner Fidget-Spinner deleted the load_attr_property branch Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon

@python/windows-team windows-team

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Fidget-Spinner @bedevere-bot @markshannon