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.

Sign up for GitHub

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-102213: Optimize the performance of __getattr__ #103761

Merged
merged 8 commits into from
May 1, 2023
Merged

gh-102213: Optimize the performance of __getattr__ #103761

merged 8 commits into from
May 1, 2023

Conversation

sunmy2019
Copy link
Member

@sunmy2019 sunmy2019 commented Apr 24, 2023 •
edited by bedevere-bot
Loading

This is the logically corrected version of GH-102248.

GH-102248 caused several issues and had been reverted. This PR (I think) correctly implements it.

Tests passed on Ubuntu 22.04 (WSL) gcc-11 with --pydebug.

I also do the perf tests in the original PR, (--enable-optimizations with my AMD 5900X)
Before,

Python version of 3.12.0a7+ (heads/gh-102248-corrected:af53046995, Apr 24 2023, 22:42:22) [GCC 11.3.0]
Call __getattr__ spend time: 0.2576024532318115

After,

Python version of 3.12.0a7+ (heads/gh-102248-corrected:ac81fd6c7e, Apr 24 2023, 22:39:39) [GCC 11.3.0]
Call __getattr__ spend time: 0.04635906219482422

cc: @Fidget-Spinner

Eclips4
Eclips4 reviewed Apr 24, 2023
View reviewed changes
Objects/typeobject.c Outdated Show resolved Hide resolved
sunmy2019
sunmy2019 commented Apr 24, 2023
View reviewed changes
Objects/typeobject.c Outdated Show resolved Hide resolved
@sunmy2019 sunmy2019 changed the title Optimize the performance of __getattr__ gh-102213: Optimize the performance of __getattr__ Apr 24, 2023
@bedevere-bot bedevere-bot mentioned this pull request Apr 24, 2023
Closed
@Fidget-Spinner
Copy link
Member

Note to myself or whoever who merges this. Please add @wangxiang-hz to co-authors when merging as this is the fixed version of their PR, and the work should also be attributed to them.

@Fidget-Spinner
Copy link
Member

I will review and hopefully merge this on Friday when I have more time. Thanks.

@Fidget-Spinner Fidget-Spinner self-assigned this Apr 24, 2023
@bluetech
Copy link

#103551 has a simple test case for the issue with the original PR, might be useful to add as a unit test for this PR.

@sunmy2019
Copy link
Member Author

#103551 has a simple test case for the issue with the original PR, might be useful to add as a unit test for this PR.

@property raises and __getattr__ not called --> added in #103358
@property calls __getattr__, and __getattr__ raises --> added in #103336

The test of #103551 should be covered by #103358. But a little duplication is okay. I will add it later.

@Fidget-Spinner Fidget-Spinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 27, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit c3aebf6 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 27, 2023
@sunmy2019
Copy link
Member Author

sunmy2019 commented Apr 27, 2023 •
edited
Loading

Does anyone have any idea why this fails?
https://buildbot.python.org/all/#/builders/738/builds/3112
The failure is a timing test. This PR seems to have changed the time usage of one particular test on ARM.

Got a timeout here: https://buildbot.python.org/all/#/builders/403/builds/1066

all builds: https://buildbot.python.org/all/#/changes/22158

@Fidget-Spinner
Copy link
Member

Got a timeout here: https://buildbot.python.org/all/#/builders/403/builds/1066

This specific test seems flaky. I would ignore it.

@sunmy2019
Copy link
Member Author

sunmy2019 commented Apr 30, 2023 •
edited
Loading

@Fidget-Spinner How is it going on? We are running out of time. New features will soon be frozen in May. 7th.
https://peps.python.org/pep-0693/

Or will this be considered as a bug fix?

@Fidget-Spinner Fidget-Spinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 30, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 75d9a70 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 30, 2023
@Fidget-Spinner
Copy link
Member

@Fidget-Spinner How is it going on? We are running out of time. New features will soon be frozen in May. 7th. https://peps.python.org/pep-0693/

Or will this be considered as a bug fix?

I'm triggering a buildbot rerun to see if that specific buildbot is actually impacted.

@sunmy2019
Copy link
Member Author

https://buildbot.python.org/all/#builders/178/builds/1189

test_monitoring failure is tracked here
#103845
test_import failure is tracked here
#103879 (comment)

Seems no more failures

@Fidget-Spinner
Copy link
Member

Please add news. You can copy the previous PR's news item. I will attribute the commit to both of you.

@Fidget-Spinner Fidget-Spinner merged commit 59c27fa into python:main May 1, 2023
@arhadthedev
Copy link
Member

Please, don't worry about 229 random files reported as changed. It's a GitHub after-merge artifact, the final commit 59c27fa squash-merged into main is fine.

carljm added a commit to carljm/cpython that referenced this pull request May 1, 2023
@carljm
Merge branch 'main' into supermon
159b2aa 
* main: (26 commits)
  pythongh-104028: Reduce object creation while calling callback function from gc (pythongh-104030)
  pythongh-104036: Fix direct invocation of test_typing (python#104037)
  pythongh-102213: Optimize the performance of `__getattr__` (pythonGH-103761)
  pythongh-103895: Improve how invalid `Exception.__notes__` are displayed (python#103897)
  Adjust expression from `==` to `!=` in alignment with the meaning of the paragraph. (pythonGH-104021)
  pythongh-88496: Fix IDLE test hang on macOS (python#104025)
  Improve int test coverage (python#104024)
  pythongh-88773: Added teleport method to Turtle library (python#103974)
  pythongh-104015: Fix direct invocation of `test_dataclasses` (python#104017)
  pythongh-104012: Ensure test_calendar.CalendarTestCase.test_deprecation_warning consistently passes (python#104014)
  pythongh-103977: compile re expressions in platform.py only if required (python#103981)
  pythongh-98003: Inline call frames for CALL_FUNCTION_EX (pythonGH-98004)
  Replace Netlify with Read the Docs build previews (python#103843)
  Update name in acknowledgements and add mailmap (python#103696)
  pythongh-82054: allow test runner to split test_asyncio to execute in parallel by sharding. (python#103927)
  Remove non-existing tools from Sundry skiplist (python#103991)
  pythongh-103793: Defer formatting task name (python#103767)
  pythongh-87092: change assembler to use instruction sequence instead of CFG (python#103933)
  pythongh-103636: issue warning for deprecated calendar constants (python#103833)
  Various small fixes to dis docs (python#103923)
  ...
carljm added a commit to carljm/cpython that referenced this pull request May 1, 2023
@carljm
Merge branch 'main' into inlinecomp2
1274e2b 
* main: (463 commits)
  pythongh-104057: Fix direct invocation of test_super (python#104064)
  pythongh-87092: Expose assembler to unit tests (python#103988)
  pythongh-97696: asyncio eager tasks factory (python#102853)
  pythongh-84436: Immortalize in _PyStructSequence_InitBuiltinWithFlags() (pythongh-104054)
  pythongh-104057: Fix direct invocation of test_module (pythonGH-104059)
  pythongh-100458: Clarify Enum.__format__() change of mixed-in types in the whatsnew/3.11.rst (pythonGH-100387)
  pythongh-104018: disallow "z" format specifier in %-format of byte strings (pythonGH-104033)
  pythongh-104016: Fixed off by 1 error in f string tokenizer (python#104047)
  pythonGH-103629: Update Unpack's repr in compliance with PEP 692 (python#104048)
  pythongh-102799: replace sys.exc_info by sys.exception in inspect and traceback modules (python#104032)
  Fix typo in "expected" word in few source files (python#104034)
  pythongh-103824: fix use-after-free error in Parser/tokenizer.c (python#103993)
  pythongh-104035: Do not ignore user-defined `__{get,set}state__` in slotted frozen dataclasses (python#104041)
  pythongh-104028: Reduce object creation while calling callback function from gc (pythongh-104030)
  pythongh-104036: Fix direct invocation of test_typing (python#104037)
  pythongh-102213: Optimize the performance of `__getattr__` (pythonGH-103761)
  pythongh-103895: Improve how invalid `Exception.__notes__` are displayed (python#103897)
  Adjust expression from `==` to `!=` in alignment with the meaning of the paragraph. (pythonGH-104021)
  pythongh-88496: Fix IDLE test hang on macOS (python#104025)
  Improve int test coverage (python#104024)
  ...
@wangxiang-hz
Copy link
Contributor

Sorry guys, I just noticed this pull request. I was buy with my work these weeks and just finished a major update at the end of April..
I think @sunmy2019 is right and his commits is better. call_attribute should only be called when attribute is not found, then we call it, and clear the PyExc_AttributeError being raised.
I made a mistake, PyObject_GenericGetAttr may not raise attribute error but other scripts may still raise other exceptions. When this happen, call_attribute should not be called.
Thank you all for finding this and sorry for the bugs triggered.

@sunmy2019 sunmy2019 deleted the gh-102248-corrected branch May 10, 2023 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

@Eclips4 Eclips4 Eclips4 left review comments

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

@Fidget-Spinner Fidget-Spinner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sunmy2019 @Fidget-Spinner @bluetech @bedevere-bot @arhadthedev @wangxiang-hz @Eclips4 @ambv