bpo-42100: Add _PyType_GetModuleByDef by encukou · Pull Request #22835 · python/cpython · GitHub

See https://mail.python.org/archives/list/capi-sig@python.org/thread/T3P2QNLNLBRFHWSKYSTPMVEIL2EEKFJU/ for discussion.

https://bugs.python.org/issue42100

See #22838 for example usage.

nit comment.

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

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++) {

Suggested change

int i;

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

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

Thanks!

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

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.

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 !

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

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.

* 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

See https://mail.python.org/archives/list/capi-sig@python.org/thread/T3P2QNLNLBRFHWSKYSTPMVEIL2EEKFJU/ for discussion. https://bugs.python.org/issue42100

the-knights-who-say-ni added the CLA signed label Oct 20, 2020

bedevere-bot added the awaiting core review label Oct 20, 2020

encukou added the skip news label Oct 20, 2020

Add _PyType_GetModuleByDef

0b0d93c

encukou force-pushed the pep-573-walker branch from 3adafc3 to 0b0d93c Compare October 20, 2020 20:54

encukou mentioned this pull request Oct 20, 2020

bpo-1635741: Port _csv to multi-phase init #22838

Closed

encukou requested review from ncoghlan and vstinner October 20, 2020 22:05

encukou mentioned this pull request Oct 21, 2020

Add a MRO walker for slot methods and Convert _csv to use PEP 573 encukou/cpython#4

Closed

corona10 reviewed Oct 21, 2020

View reviewed changes

encukou requested a review from ericsnowcurrently October 23, 2020 15:21

shihai1991 approved these changes Oct 31, 2020

View reviewed changes

encukou merged commit 57aaaa8 into python:master Nov 3, 2020

bedevere-bot removed the awaiting core review label Nov 3, 2020

encukou deleted the pep-573-walker branch November 3, 2020 21:27

vstinner mentioned this pull request Nov 4, 2020

bpo-40077: Convert arraymodule to use heap types and establish module state #23124

Merged

bpo-42100: Add _PyType_GetModuleByDef by encukou · Pull Request #22835 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-42100: Add _PyType_GetModuleByDef - #22835

bpo-42100: Add _PyType_GetModuleByDef#22835
encukou merged 1 commit into
python:masterfrom
encukou:pep-573-walker

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

Uh oh!

Uh oh!

encukou commented Oct 20, 2020

Uh oh!

corona10 left a comment

Uh oh!

corona10 Oct 21, 2020

Uh oh!

corona10 Oct 21, 2020

Uh oh!

encukou commented Oct 26, 2020

Uh oh!

shihai1991 left a comment

Uh oh!

encukou commented Nov 3, 2020

Uh oh!

vstinner commented Nov 4, 2020

Uh oh!

vstinner commented Nov 4, 2020

Uh oh!

vstinner commented Nov 4, 2020

Uh oh!

encukou commented Nov 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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

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

bpo-42100: Add _PyType_GetModuleByDef by encukou · Pull Request #22835 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-42100: Add _PyType_GetModuleByDef - #22835

bpo-42100: Add _PyType_GetModuleByDef#22835encukou merged 1 commit intopython:masterpython/cpython:masterfrom encukou:pep-573-walkerencukou/cpython:pep-573-walkerCopy head branch name to clipboard

Conversation

encukou commented Oct 20, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou commented Oct 20, 2020

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

encukou commented Oct 26, 2020

Uh oh!

shihai1991 left a comment

Choose a reason for hiding this comment

Uh oh!

encukou commented Nov 3, 2020

Uh oh!

vstinner commented Nov 4, 2020

Uh oh!

vstinner commented Nov 4, 2020

Uh oh!

vstinner commented Nov 4, 2020

Uh oh!

encukou commented Nov 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-42100: Add _PyType_GetModuleByDef#22835
encukou merged 1 commit into
python:masterfrom
encukou:pep-573-walker

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