GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-42100: Add _PyType_GetModuleByDef by encukou · Pull Request #22835 · python/cpython

Conversation

@encukou

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

See https://mail.python.org/archives/list/[email protected]/thread/T3P2QNLNLBRFHWSKYSTPMVEIL2EEKFJU/ for discussion.

https://bugs.python.org/issue42100

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 20, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Oct 20, 2020
@encukou encukou added the skip news label Oct 20, 2020
@encukou
Add _PyType_GetModuleByDef
0b0d93c
@encukou
encukou force-pushed the pep-573-walker branch from 3adafc3 to 0b0d93c Compare October 20, 2020 20:54
@encukou encukou mentioned this pull request Oct 20, 2020
bpo-1635741: Port _csv to multi-phase init #22838
Closed
@encukou

encukou commented Oct 20, 2020

Copy link
Copy Markdown
Member Author

See #22838 for example usage.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou
encukou requested review from ncoghlan and vstinner October 20, 2020 22:05
@encukou 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
corona10 reviewed Oct 21, 2020
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/typeobject.c
assert(PyType_Check(type));
assert(type->tp_mro);
int i;
for (i = 0; i < PyTuple_GET_SIZE(type->tp_mro); i++) {

@corona10 corona10 Oct 21, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/typeobject.c
{
assert(PyType_Check(type));
assert(type->tp_mro);
int i;

@corona10 corona10 Oct 21, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
int i;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

encukou commented Oct 26, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shihai1991
shihai1991 approved these changes Oct 31, 2020
View reviewed changes

@shihai1991 shihai1991 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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~

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@encukou
encukou merged commit 57aaaa8 into python:master Nov 3, 2020
@bedevere-bot bedevere-bot removed the awaiting core review label Nov 3, 2020
@encukou
encukou deleted the pep-573-walker branch November 3, 2020 21:27
@encukou

encukou commented Nov 3, 2020

Copy link
Copy Markdown
Member Author

Thanks!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 4, 2020

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 4, 2020

Copy link
Copy Markdown
Member

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 !

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Nov 4, 2020
bpo-40077: Convert arraymodule to use heap types and establish module state #23124
Merged
@vstinner

vstinner commented Nov 4, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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/[email protected]/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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@encukou @vstinner @corona10 @shihai1991 @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.