NotificationsYou must be signed in to change notification settings
Fork33.4k
Star69.8k

bpo-1635741: port _elementtree to multi-phase init (PEP 489)#23535

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

koubaa wants to merge2 commits intopython:mainfromkoubaa:bpo-1635741-elementtree

Closed

bpo-1635741: port _elementtree to multi-phase init (PEP 489)#23535

koubaa wants to merge2 commits intopython:mainfromkoubaa:bpo-1635741-elementtree

Conversation

Copy link

Contributor

koubaa commentedNov 28, 2020•
edited by bedevere-bot
Loading

https://bugs.python.org/issue1635741

post _elementtree to multi-phase init

d5b1fb6

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

Nov 28, 2020

bedevere-bot added the awaiting review label

Nov 28, 2020

Copy link

ContributorAuthor

koubaa commentedNov 28, 2020

@vstinner @shihai1991 @corona10 please review

tiran requested changes

Nov 28, 2020

View reviewed changes

Copy link

Member

tiran left a comment

Choose a reason for hiding this comment

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

The patch looks like it was a lot of work. Thanks for investing your time to port_elementtree.

I'm a bit worried that the module state getter is going to affect performance negatively. The code has to use the slow_PyType_GetModuleByDef() API. In my patch for the_ssl module I added a module state pointer to instance structs. For example you could addstate totypedef struct ElementObject and then reference the state withself->state. This would reduce the amount of get state calls and simplify the code.

What do you think? Is this a safe apprach?

Modules/_elementtree.c

		#defineET_STATE_GLOBAL \
		((elementtreestate *) PyModule_GetState(PyState_FindModule(&elementtreemodule)))
		staticinlineelementtreestate*
		get_elementtree_module_state_by_object(void*object)

Copy link

Member

tiranNov 28, 2020

Choose a reason for hiding this comment

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

The qualifierobject is not self-explanatory. Every Python object is an object. This includes module objects and type objects. How about:

Suggested change

	get_elementtree_module_state_by_object(void*object)
	get_elementtree_module_state_by_instance(PyObject*self)

Copy link

Member

shihai1991Nov 28, 2020

Choose a reason for hiding this comment

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

How about expendget_element_module_state() to receiveself object ortype object?

Copy link

ContributorAuthor

koubaaNov 28, 2020

Choose a reason for hiding this comment

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

@tiran I tried usingPyObject * and that gives a lot of compiler warnings (see somem below):

C:\Dev\OSS\cpython\cpython\Modules_elementtree.c(3749,74): warning C4133: 'function': incompatible types - from 'XMLParserObject *' to 'PyObject *' [C:\Dev\OSS\cpython\cpython\PCbuild_elementtree.vcxproj]
C:\Dev\OSS\cpython\cpython\Modules\clinic_elementtree.c.h(26,52): warning C4133: 'function': incompatible types - from 'ElementObject *' to 'PyObject *' [C:\Dev\OSS\cpython\cpython\PCbuild_elementtree.vcxproj]

Copy link

ContributorAuthor

koubaaNov 28, 2020

Choose a reason for hiding this comment

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

@shihai1991 do you mean two methods, one for each?

Copy link

Member

shihai1991Nov 30, 2020

Choose a reason for hiding this comment

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

yes. just my personal suggestion :)

Modules/_elementtree.cShow resolvedHide resolved

Modules/_elementtree.c

Comment on lines +384 to +386

		class _elementtree.Element "ElementObject *" "get_elementtree_module_state_by_object(self)->element_type"
		class _elementtree.TreeBuilder "TreeBuilderObject *" "get_elementtree_module_state_by_object(self)->tree_builder_type"
		class _elementtree.XMLParser "XMLParserObject *" "get_elementtree_module_state_by_object(self)->xml_parser_type"

Copy link

Member

tiranNov 28, 2020

Choose a reason for hiding this comment

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

Is this correct? AFAIK the argument clinic code has only access totype, not toself.

Copy link

ContributorAuthor

koubaaNov 28, 2020

Choose a reason for hiding this comment

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

@tiran This is where I got from trial and error, see the implementation of _elementtree_Element_append in Modules/clinic/_elementtree.c.h

bedevere-bot added awaiting changes and removed awaiting review labels

Nov 28, 2020

Copy link

bedevere-bot commentedNov 28, 2020

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Copy link

ContributorAuthor

koubaa commentedNov 28, 2020

The patch looks like it was a lot of work. Thanks for investing your time to port_elementtree.
I'm a bit worried that the module state getter is going to affect performance negatively. The code has to use the slow_PyType_GetModuleByDef() API. In my patch for the_ssl module I added a module state pointer to instance structs. For example you could addstate totypedef struct ElementObject and then reference the state withself->state. This would reduce the amount of get state calls and simplify the code.
What do you think? Is this a safe apprach?

@tiran I'm not sure if its safe but I understand it in principle. I don't know exactly the lifetime of instance structs when pickling.@vstinner thoughts on that approach?

Is there a benchmark I could try to measure the performance impact? _PyType_GetModuleByDef seems to only be slow if there is significant subclassing so I could try to whip up an example that tries to overdo that.

visit/clear

8f2355a

Copy link

Contributor

erlend-aasland commentedDec 1, 2020

This PR introduces a ref. leak (looks similar to the ref. leak I had in PR#23428):

bash$ ./python.exe -m test -F -r -j1 -R 3:5 test_xml_etree_cUsing random seed 17428180:00:00 load avg: 1.54 Run tests in parallel using 1 child processes0:00:14 load avg: 1.50 [  1/1] test_xml_etree_c failedbeginning 8 repetitions12345678........test_xml_etree_c leaked [147, 147, 147, 147, 147] references, sum=735test_xml_etree_c leaked [59, 59, 59, 59, 59] memory blocks, sum=295== Tests result: FAILURE ==1 test failed:    test_xml_etree_cTotal duration: 14.8 secTests result: FAILURE

Copy link

ContributorAuthor

koubaa commentedDec 11, 2020

@erlend-aasland oof sorry, I didn't realize you were working on this module too. Any tips to find the cause of the ref leak or which reference is leaking? I'm not too familiar with the diagnostics & telemetry available.

Copy link

Contributor

erlend-aasland commentedDec 11, 2020•
edited
Loading

oof sorry, I didn't realize you were working on this module too.

No problem, you're fine! Feel free to take a look at my PR. I haven't been able to devote any free time to this the last month or so, anyway.

Any tips to find the cause of the ref leak or which reference is leaking? I'm not too familiar with the diagnostics & telemetry available.

Me neither. It kinda stopped when I encountered the ref. leak. I usepython.exe -m test -F -r -j1 -R 3:5 test_<mod> to check for reference/memory leaks. I've tried playing with some of the-X options as well, but I haven't found a good way to debug ref leaks yet. I'll keep an eye on this PR to see what you find out :)

Copy link

github-actionsbot commentedJan 11, 2021

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

github-actionsbot added the staleStale PR or inactive for long period of time. label

Jan 11, 2021

Copy link

Member

iritkatriel commentedApr 11, 2022

https://bugs.python.org/issue1635741 is closed. What is that status of this PR?

Copy link

Member

vstinner commentedApr 19, 2022

The change is still relevant, but should use a new issue number.

Moreover, the SC asked to put the conversion of static types to heap types on hold.@encukou and@erlend-aasland wrotehttps://peps.python.org/pep-0687/ which may unblock the situation but it's still a draft.