8000 GH-92123: Move _elementtree heap types to module state by erlend-aasland · Pull Request #101187 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

GH-92123: Move _elementtree heap types to module state #101187

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
Merged
merged 28 commits into from
Jan 21, 2023
Merged

GH-92123: Move _elementtree heap types to module state #101187

merged 28 commits into from
Jan 21, 2023

Conversation

erlend-aasland
Copy link
Contributor
@erlend-aasland erlend-aasland commented Jan 20, 2023 •
edited
Loading

Erlend E. Aasland and others added 24 commits January 11, 2021 09:43
Prepare init function
76641a3
Convert element type to heap type
5cbbe16
Convert element iter type to heap type
116fa39
Convert tree builder type to heap type
a792fcf
Convert xml parser type to heap type
341a92d
Update clinic
f35c604
Add NEWS
b6573b1
Use PyModule_AddObjectRef
404b398
Create module before creating types
6e53e92
Fetch type before GC
525cfaf
Add initialisation guard
2448e93
Address Heime's review: Don't define globals twice
959a225
Visit type in type traverse functions
9210a36
Fix NEWS formatting
a6c6fea
@erlend-aasland
Sync with main
e039922
@erlend-aasland
Sync with main
b02b18e
@erlend-aasland
Merge branch 'main' into isolate-elementtree
744be65
@erlend-aasland
Address review: make types immutable
c8a3c67
@erlend-aasland
Disallow instantiation for the iter type
2175f66
@erlend-aasland
PEP 687: Move types to state struct
1ef41e1
@erlend-aasland
Sync with main
dbc4417
@erlend-aasland
refactor: replace query with parameter
24794dd 
Pass state as an argument to Element_CheckExact()
@erlend-aasland
refactor: replace query with parameter
6bb6511 
Pass state as an argument to Element_Check()
@erlend-aasland
refactor: replace query with parameter
8dccc5a 
Pass state as an argument to TreeBuilder_CheckExact()
@bedevere-bot bedevere-bot mentioned this pull request Jan 20, 2023
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jan 20, 2023
Merged
@erlend-aasland erlend-aasland mentioned this pull request Jan 20, 2023
Merged
kumaraditya303
kumaraditya303 reviewed Jan 21, 2023
View reviewed changes
@erlend-aasland @kumaraditya303
Update Modules/_elementtree.c
78d1676 
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
arhadthedev
arhadthedev reviewed Jan 21, 2023
View reviewed changes
@erlend-aasland
Copy link
Contributor Author
erlend-aasland commented Jan 21, 2023 •
edited
Loading

Thanks for the reviews, Kumar and Oleg!

For the path forward, there are two options: either pass state as a parameter, or store it in the type structs. I've created draft PRs for the two alternatives. Of course, a third path is a combination of both.

@erlend-aasland erlend-aasland merged commit 13566a3 into python:main Jan 21, 2023
@erlend-aasland erlend-aasland deleted the etree/move-heap-types-to-state branch January 21, 2023 11:01
@arhadthedev
Copy link
Member

For the path forward, there are three options: either pass state as a parameter, store it in the type structs.

Both variants ultimately call PyModule_GetState, right? [with (1) caching its result into a field of self and (2) plainly calling it each time on demand]

If so, we need an informed decision by benchmarking some _elementtree method in both options. It should show whether direct structure access of (1) is faster than preheated call to PyModule_GetState of (2).

@erlend-aasland
Copy link
Contributor Author
erlend-aasland commented Jan 21, 2023 •
edited
Loading

Both variants ultimately call PyModule_GetState, right?

PyModule_GetState is called for module level functions, PyType_GetModuleState for class methods (via defining class), and PyType_GetModuleByDef plus PyModule_GetState for slots. The two former are inexpensive lookups, the latter is slightly slower. Caching state in self results in more memory usage (might only be important for the Element type). I'd be surprised if it is possible to measure the performance impact of this, but sure let's do some benchmarking.

@erlend-aasland
Copy link
Contributor Author

I ran the pyperformance xml_etree benchmark towards all three alternatives (1: pass state around as a parameter, 2: store state in the heap type contexts, 3: store state in TreeBuilder and XMLParser contexts, but pass it around for Element and its iter). I could not measure a significant difference any of the alternatives.

@kumaraditya303
Copy link
Contributor

It won't matter as expected as most of the time as long as there isn't much stack spilling the performance should be identical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arhadthedev arhadthedev arhadthedev left review comments

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@erlend-aasland @arhadthedev @kumaraditya303 @bedevere-bot
0