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-40077: Convert _elementtree types to heap types by erlend-aasland · Pull Request #23428 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Nov 20, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue40077

Sorry, something went wrong.

Uh oh!

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

All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 20, 2020
@bedevere-bot bedevere-bot added the awaiting review label Nov 20, 2020
@erlend-aasland erlend-aasland changed the title bpo40077: Convert _elementtree types to heap types bpo-40077: Convert _elementtree types to heap types Nov 20, 2020
@erlend-aasland

erlend-aasland commented Nov 20, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@corona10 would you mind taking a look at this, when I've sorted out the CI failures?

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
erlend-aasland marked this pull request as draft November 20, 2020 20:31
tiran
tiran reviewed Nov 21, 2020
View reviewed changes
Comment thread Modules/_elementtree.c Resolved

Uh oh!

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

@tiran

tiran commented Nov 21, 2020

Copy link
Copy Markdown
Member

Tests are failing because subinterpreter re-initialized etree types. You either have to add an initialized guard to type initialization or move the types to module state completely.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Nov 21, 2020

Copy link
Copy Markdown
Contributor Author

Tests are failing because subinterpreter re-initialized etree types. You either have to add an initialized guard to type initialization or move the types to module state completely.

Ah, thanks, @tiran! I modified the CREATE_TYPE macro with a simple guard. Moving to module state (and multi-phase init) will also be a large PR, so for the reviewers convenience, I think it's best to keep those changes separate.

All reactions

Sorry, something went wrong.

Uh oh!

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

tiran
tiran reviewed Nov 22, 2020
View reviewed changes
Comment thread Modules/_elementtree.c Resolved

Uh oh!

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

@vstinner vstinner mentioned this pull request Nov 23, 2020
bpo-42327: Add PyModule_Add() (smaller). #23443
Merged
@erlend-aasland
erlend-aasland force-pushed the bpo-40077/elementtree branch from 9d7b90b to 5f584aa Compare November 23, 2020 11:10
@erlend-aasland erlend-aasland mentioned this pull request Dec 1, 2020
bpo-1635741: port _elementtree to multi-phase init (PEP 489) #23535
Closed
@github-actions

github-actions Bot commented Dec 24, 2020

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 24, 2020
Erlend E. Aasland added 13 commits January 11, 2021 09:43
Erlend E. Aasland
Prepare init function
76641a3
Erlend E. Aasland
Convert element type to heap type
5cbbe16
Erlend E. Aasland
Convert element iter type to heap type
116fa39
Erlend E. Aasland
Convert tree builder type to heap type
a792fcf
Erlend E. Aasland
Convert xml parser type to heap type
341a92d
Erlend E. Aasland
Update clinic
f35c604
Erlend E. Aasland
Add NEWS
b6573b1
Erlend E. Aasland
Use PyModule_AddObjectRef
404b398
Erlend E. Aasland
Create module before creating types
6e53e92
Erlend E. Aasland
Fetch type before GC
525cfaf
Erlend E. Aasland
Add initialisation guard
2448e93
Erlend E. Aasland
Address Heime's review: Don't define globals twice
959a225
Erlend E. Aasland
Visit type in type traverse functions
9210a36
@erlend-aasland
erlend-aasland force-pushed the bpo-40077/elementtree branch from 5f584aa to 9210a36 Compare January 11, 2021 08:53
@erlend-aasland

erlend-aasland commented Jan 11, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Ref. leaks are now fixed (commit 9210a36):

$ ./python.exe -m test -R 3:5 test_xml_etree test_xml_etree_c                                                                                                         ([bpo-40077](https://bugs.python.org/issue40077)/elementtree)cpython.git
0:00:00 load avg: 1.25 Run tests sequentially
0:00:00 load avg: 1.25 [1/2] test_xml_etree
beginning 8 repetitions
12345678
........
0:00:03 load avg: 1.39 [2/2] test_xml_etree_c
beginning 8 repetitions
12345678
........

== Tests result: SUCCESS ==

All 2 tests OK.

Total duration: 12.4 sec
Tests result: SUCCESS

Rebased onto master & force-pushed; hope that's ok. Ready for review, @tiran.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
erlend-aasland marked this pull request as ready for review January 11, 2021 09:10
Erlend E. Aasland
Fix NEWS formatting
a6c6fea
@erlend-aasland
erlend-aasland requested a review from tiran January 11, 2021 09:14
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jan 12, 2021
@erlend-aasland

erlend-aasland commented Feb 1, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

FYI, all of these types follow the GC protocol, as described in bpo-42972.

(cc. @vstinner @shihai1991)

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Feb 1, 2021

Copy link
Copy Markdown
Member

I dislike the temporary situation where we create new heap types but don't clear them at exit.

Would it be possible to write a first PR to convert the _elementtree extension to multiphase init? And maybe also prepare the code for having per module types (having a module state)? (same PR or a different PR) For example, create a state which contains pointers to the static types, and ensure that static types are no longer referenced directly.

shihai1991 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Feb 1, 2021

Copy link
Copy Markdown
Contributor Author

I dislike the temporary situation where we create new heap types but don't clear them at exit.

I agree.

Would it be possible to write a first PR to convert the _elementtree extension to multiphase init? And maybe also prepare the code for having per module types (having a module state)? (same PR or a different PR) For example, create a state which contains pointers to the static types, and ensure that static types are no longer referenced directly.

No problem. I'll create a new PR for multiphase init (bpo 1635741). If it's not too large a change, I'll include module state as well.

shihai1991 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
erlend-aasland marked this pull request as draft February 1, 2021 22:23
@erlend-aasland

erlend-aasland commented Feb 5, 2021

Copy link
Copy Markdown
Contributor Author

Would it be possible to write a first PR to convert the _elementtree extension to multiphase init?

I did some fiddling with this today, and the problem is that we've already got a (partial) module state, and it often uses PyState_FindModule (via the ET_STATE_GLOBAL macro) to look up the module, in order to fetch the state. Since we can't use PyState_FindModule with multi-phase init, there are two solutions, as I see it:

  1. Fetch the state using _PyType_GetModuleByDef and PyType_GetModule, but that requires converting the static types to heap types => the diff explodes => difficult to review
  2. (Temporary) convert the existing module state to a global static state, and modify ET_STATE_GLOBAL and get_elementtree_state to return a pointer to the static state. Pro: small PR, easy to review. Con: A small step backwards, module state wise.

What do you think?

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland closed this Mar 19, 2021
@erlend-aasland

erlend-aasland commented Jun 27, 2022

Copy link
Copy Markdown
Contributor Author

FYI, PEP-687 was just accepted.

All reactions

Sorry, something went wrong.

Uh oh!

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

@corona10 corona10 mentioned this pull request Jun 27, 2022
Convert static types to heap types: use PyType_FromSpec() #84258
Closed
@erlend-aasland erlend-aasland mentioned this pull request Nov 7, 2022
gh-92123: Convert _elementtree types to heap types #99221
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiran tiran Awaiting requested review from tiran

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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.

5 participants

@erlend-aasland @tiran @vstinner @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.