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

bpo-42100: Add _PyType_GetModuleByDef #22835

Merged
merged 1 commit into from
Nov 3, 2020
Merged

bpo-42100: Add _PyType_GetModuleByDef #22835

merged 1 commit into from
Nov 3, 2020

Conversation

encukou
Copy link
Member

@encukou encukou commented Oct 20, 2020 •
edited by bedevere-bot
Loading

@encukou encukou mentioned this pull request Oct 20, 2020
Closed
@encukou
Copy link
Member Author

encukou commented Oct 20, 2020

See #22838 for example usage.

@encukou encukou mentioned this pull request Oct 21, 2020
Closed
corona10
corona10 reviewed Oct 21, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

nit comment.

IMHO, we can use C99 features since some of the module already use other c99 features.
(for example, struct init)

Objects/typeobject.c
assert(PyType_Check(type));
assert(type->tp_mro);
int i;
for (i = 0; i < PyTuple_GET_SIZE(type->tp_mro); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (i = 0; i < PyTuple_GET_SIZE(type->tp_mro); i++) {
for (int i = 0; i < PyTuple_GET_SIZE(type->tp_mro); i++) {

Objects/typeobject.c
{
assert(PyType_Check(type));
assert(type->tp_mro);
int i;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int i;

@encukou
Copy link
Member Author

encukou commented Oct 26, 2020

IMO that's a code style issue, and PEP7 has nothing against it. I'd rather keep it as is.
The declaration is already C99-style: it's not at the beginning of the function :)

shihai1991
shihai1991 approved these changes Oct 31, 2020
View reviewed changes
Copy link
Member

@shihai1991 shihai1991 left a comment

Choose a reason for hiding this comment

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

Thanks, petr. I have no any other comment in here.
PS: I have reviewed it months ago in fork repo~

@encukou encukou merged commit 57aaaa8 into python:master Nov 3, 2020
@encukou encukou deleted the pep-573-walker branch November 3, 2020 21:27
@encukou
Copy link
Member Author

encukou commented Nov 3, 2020

Thanks!

And thanks for the other review – this PR includes the changes you pointed out :)

@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

When a type has no subclass but a long MRO, _PyType_GetModuleByDef() has to iterate on the N parent classes before reaching the last one which will match. Would it be more efficient to iterator on the MRO in the reverse order?

It would make the function slower for subclasses, but faster for direct instance of the type.

@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

Usually, when I read "get", I expect a O(1) operation. I'm fine if the first call fills a cache and is slower. But here, every call has a complexity of O(n) where n is the length of the MRO tuple. Would it make sense to rename the function to _PyType_FindModuleByDef() to announce that it can be slow?

Anyway, thanks for adding this function! It will unblock porting many extension modules to multi-phase init and heap types in https://bugs.python.org/issue1635741 !

@vstinner vstinner mentioned this pull request Nov 4, 2020
Merged
@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

About _PyType_FindModuleByDef() name, here is a more concrete example: the PR #23124 converts the array extension to multi-phase init. The current PR defines:

#define get_array_state_by_type(tp) \
    (get_array_state(_PyType_GetModuleByDef(tp, &arraymodule)))
#define get_array_state_by_class(cls) \
    (get_array_state(PyType_GetModule(cls)))

I would prefer to use "find" in the first macro, to better highlight that "get_array_state_by_type()" is a fast attribute access:

#define find_array_state_by_type(tp) \
    (get_array_state(_PyType_FindModuleByDef(tp, &arraymodule)))
#define get_array_state_by_type(cls) \
    (get_array_state(PyType_GetModule(cls)))

@encukou
Copy link
Member Author

encukou commented Nov 4, 2020

Usually, when I read "get", I expect a O(1) operation.

Hm, I don't expect that. "get" can mean many things :)

I'll keep it in mind for if/when it becomes public API. I don't think it's worth changing the name now, after all the discussion.

shihai1991 added a commit to shihai1991/cpython that referenced this pull request Nov 5, 2020
@shihai1991
Merge branch 'master' into bpo_41832
c267e35 
* master:
  bpo-42260: Add _PyInterpreterState_SetConfig() (pythonGH-23158)
  Disable peg generator tests when building with PGO (pythonGH-23141)
  bpo-1635741: _sqlite3 uses PyModule_AddObjectRef() (pythonGH-23148)
  bpo-1635741: Fix PyInit_pyexpat() error handling (pythonGH-22489)
  bpo-42260: Main init modify sys.flags in-place (pythonGH-23150)
  bpo-1635741: Fix ref leak in _PyWarnings_Init() error path (pythonGH-23151)
  bpo-1635741: _ast uses PyModule_AddObjectRef() (pythonGH-23146)
  bpo-1635741: _contextvars uses PyModule_AddType() (pythonGH-23147)
  bpo-42260: Reorganize PyConfig (pythonGH-23149)
  bpo-1635741: Add PyModule_AddObjectRef() function (pythonGH-23122)
  bpo-42236: os.device_encoding() respects UTF-8 Mode (pythonGH-23119)
  bpo-42251: Add gettrace and getprofile to threading (pythonGH-23125)
  Enable signing of nuget.org packages and update to supported timestamp server (pythonGH-23132)
  Fix incorrect links in ast docs (pythonGH-23017)
  Add _PyType_GetModuleByDef (pythonGH-22835)
  Post 3.10.0a2
  bpo-41796: Call _PyAST_Fini() earlier to fix a leak (pythonGH-23131)
  bpo-42249: Fix writing binary Plist files larger than 4 GiB. (pythonGH-23121)
  bpo-40077: Convert mmap.mmap static type to a heap type (pythonGH-23108)
  Python 3.10.0a2
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@encukou @adorilson
Add _PyType_GetModuleByDef (pythonGH-22835)
9436820 
See https://mail.python.org/archives/list/capi-sig@python.org/thread/T3P2QNLNLBRFHWSKYSTPMVEIL2EEKFJU/ for discussion.

https://bugs.python.org/issue42100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corona10 corona10 corona10 left review comments

@shihai1991 shihai1991 shihai1991 approved these changes

@vstinner vstinner Awaiting requested review from vstinner

@ncoghlan ncoghlan Awaiting requested review from ncoghlan

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

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

Successfully merging this pull request may close these issues.
6 participants
@encukou @vstinner @corona10 @shihai1991 @the-knights-who-say-ni @bedevere-bot