GeezerVison

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

GeezerVison is showing github.com — bpo-40077: Convert _abc module to use PyType_FromSpec() by corona10 · Pull Request #19202 · python/cpython

Conversation

corona10 commented Mar 28, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Replace statically allocated types with heap allocated types:
use PyType_FromSpec().

Add a module state to store the _abc_data_type..
Add traverse, clear and free functions to the module.

https://bugs.python.org/issue40077

Sorry, something went wrong.

Uh oh!

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

All reactions


          bpo-40077: Convert _abc module to use PyType_FromSpec()

47eb700

Replace statically allocated types with heap allocated types:
use PyType_FromSpec().

Add a module state to store the _abc_data_type..
Add traverse, clear and free functions to the module.

the-knights-who-say-ni added the CLA signed label

Mar 28, 2020

bedevere-bot added the awaiting review label

Mar 28, 2020

corona10 commented

Mar 28, 2020

View reviewed changes

Comment thread

Modules/_abc.c

		negative cache to be cleared before its next use.
		Note: this counter is private. Use `abc.get_cache_token()` for
		external code. */
		static unsigned long long abc_invalidation_counter = 0;

corona10 Mar 28, 2020

Copy link

Copy Markdown

Member Author

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.

I am not sure that the abc_invalidation_counter is okay to be shared as static from interpreter Isolation point of view.

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 Mar 28, 2020

Copy link

Copy Markdown

Member Author

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.

Should we move this into PyInterpreterState?

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Mar 28, 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.

It seems like it would be safe to put it in the module state. The purpose of the counter is to invalidate the module cache. Registering a class in module instance 1 should not invalidate module instance 2. What do you think?

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 Mar 29, 2020

Copy link

Copy Markdown

Member Author

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.

I agree, I 've updated the PR.

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 requested a review from vstinner

March 28, 2020 05:47


          bpo-40077: rename

corona10 commented

Mar 29, 2020

View reviewed changes

Comment thread

Lib/test/test_abc.py Outdated

corona10 Mar 29, 2020

Copy link

Copy Markdown

Member Author

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.

This test might be what we wanted exactly.
I just updated the test for more validation.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner reviewed

Mar 29, 2020

View reviewed changes

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

It seems like you cannot access to the module state in abc_data_new() which prevents you to move abc_invalidation_counter into the module state :-(

I suggest to leave abc_invalidation_counter as a "static" variable, but please add a FIXME like:

/* FIXME: PEP 573: Move abc_invalidation_counter into _abcmodule_state */

It's safe to keep it as a static variable until each interpreter gets its own GIL.

Until it happens, the change should move the module closer to the goal of isolated interpreters.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_abc.c Resolved

Uh oh!

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

corona10 added 2 commits

March 30, 2020 00:51


          bpo-40077: Update test_abc.py

ec8ccc6


          bpo-40077: Add comment for abc_invalidation_counter.

c2ade98

corona10 force-pushed the bpo-40077-abc branch from 29dabe8 to c2ade98 Compare

March 29, 2020 15:53

corona10 requested a review from vstinner

March 29, 2020 16:08

corona10 commented

Mar 29, 2020

View reviewed changes

corona10 left a comment

Copy link

Copy Markdown

Member Author

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.

@vstinner
Thanks for the review.

PEP 573: Move abc_invalidation_counter into _abcmodule_state.

I agree, PEP 573 will solve this issue.
I 've updated the PR. Please take a look.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner reviewed

Mar 29, 2020

View reviewed changes

Comment thread

Modules/_abc.c Resolved

Uh oh!

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

Comment thread

Misc/NEWS.d/next/Core and Builtins/2020-03-28-14-28-46.bpo-40077.4C_xJm.rst Resolved

Uh oh!

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

corona10 and others added 2 commits

March 30, 2020 11:29


          Update Modules/_abc.c

0c0a895

Co-Authored-By: Victor Stinner <[email protected]>


          bpo-40077: Remove NEWS.d

32eaf52

corona10 added the skip news label

Mar 30, 2020

corona10 commented Mar 30, 2020

Copy link

Copy Markdown

Member Author

@vstinner Updated! Please take a look

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Mar 30, 2020

View reviewed changes

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

LGTM. Sorry, just one last change request, I promise ;-)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_abc.c Resolved

Uh oh!

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


          bpo-40077: Apply code review.

98b9300

vstinner reviewed

Mar 30, 2020

View reviewed changes

Comment thread

Modules/_abc.c Resolved

Uh oh!

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


          bpo-40077: Update the type of _abc_data_type.

14c3e52

vstinner merged commit 53e4c91 into python:master

Mar 30, 2020

bedevere-bot removed the awaiting review label

Mar 30, 2020

vstinner commented Mar 30, 2020

Copy link

Copy Markdown

Member

Thanks, I merged your PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

corona10 mentioned this pull request

Jun 27, 2022

Convert static types to heap types: use PyType_FromSpec() #84258

Closed

Reviewers

vstinner

vstinner left review comments

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.

4 participants

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.

GeezerVison

Conversation

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

Uh oh!

Uh oh!

corona10 Mar 28, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 28, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 28, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

corona10 commented Mar 30, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Mar 30, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

corona10 commented Mar 28, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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