Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-92123: Convert _elementtree types to heap types #99221

Merged
merged 22 commits into from
Jan 20, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Nov 7, 2022

@kumaraditya303
Copy link
Contributor

With #100689 the capsule also needs to isolated. LMK when this goes out of draft.

@erlend-aasland
Copy link
Contributor Author

With #100689 the capsule also needs to isolated. LMK when this goes out of draft.

Yes, I also made a note of that. I hope to be able to allocate time for CPython this weekend.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 12, 2023

I remember doing a proof-of-concept full isolation of _elementtree some years ago, and the diff ended up close to 1000 lines. I suggest breaking this up in several PRs. Approximately something like this:

  1. since there's already a module state, we should start by getting rid of ET_STATE_GLOBAL (large diff)
  2. then, implement multi-phase init (small diff)
  3. then, we can pull those changes into this PR, and we should probaly be good to go

We might want to split the ET_STATE_GLOBAL-thing in multiple PRs. It's bound to end up ugly.

UDPATE

This plan won't work out, because we need heap types before being able to completely get rid of ET_STATE_GLOBAL. We also need to get rid of ET_STATE_GLOBAL in order to implement multi-phase init. Things are entwined. Alternative plan:

  1. land this PR
  2. move the heap types into state struct (short PR; easy to review)
  3. get rid of ET_STATE_GLOBAL (possibly multiple PRs; this part is messy)
  4. implement multi-phase init (short PR; easy to review)

@erlend-aasland erlend-aasland marked this pull request as ready for review January 18, 2023 12:46
@erlend-aasland
Copy link
Contributor Author

@kumaraditya303: I wonder if we should tie this PR to gh-92123, or if we should create a targeted issue specifically for isolating _elementtree.

@erlend-aasland erlend-aasland changed the title WIP: Isolate elementtree PEP 687: Convert _elementtree types to heap types Jan 18, 2023
@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 18, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 744be65 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 18, 2023
@kumaraditya303
Copy link
Contributor

I wonder if we should tie this PR to #92123, or if we should create a targeted issue specifically for isolating _elementtree.

Let's link it to the bug report, we did a similar thing for asyncio IIRC.

I am in favor of landing this PR and continue other improvements in smaller PRs.

Modules/_elementtree.c Outdated Show resolved Hide resolved
Modules/_elementtree.c Outdated Show resolved Hide resolved
Modules/_elementtree.c Outdated Show resolved Hide resolved
Modules/_elementtree.c Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland changed the title PEP 687: Convert _elementtree types to heap types gh-92123: Convert _elementtree types to heap types Jan 19, 2023
@erlend-aasland
Copy link
Contributor Author

@kumaraditya303: thanks for the immutable types reminder; I also had forgotten to disallow instantiation for the iter type.

@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 19, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 04aaa3d 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 19, 2023
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@erlend-aasland
Copy link
Contributor Author

All right, let's land this; I'll open PRs for getting rid of ET_STATE_GLOBAL right away.

@erlend-aasland erlend-aasland merged commit 3847a6c into python:main Jan 20, 2023
@erlend-aasland erlend-aasland deleted the isolate-elementtree branch January 20, 2023 11:40
@erlend-aasland
Copy link
Contributor Author

Thanks for the great review, Kumar!

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM Raspbian 3.x has failed when building commit 3847a6c.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/424/builds/3305) and take a look at the build logs.
  4. Check if the failure is related to this commit (3847a6c) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/424/builds/3305

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

411 tests OK.

10 slowest tests:

  • test_venv: 6 min 29 sec
  • test_largefile: 5 min 40 sec
  • test_asyncio: 4 min 39 sec
  • test_math: 3 min 58 sec
  • test_multiprocessing_spawn: 3 min 16 sec
  • test_concurrent_futures: 3 min 15 sec
  • test_lib2to3: 2 min 31 sec
  • test_sax: 2 min 20 sec
  • test_multiprocessing_forkserver: 2 min 14 sec
  • test_tokenize: 2 min 1 sec

1 test altered the execution environment:
test_asyncio

21 tests skipped:
test_check_c_globals test_devpoll test_idle test_ioctl test_kqueue
test_launcher test_msilib test_peg_generator test_perf_profiler
test_startfile test_tcl test_tix test_tkinter test_ttk
test_ttk_textonly test_turtle test_winconsoleio test_winreg
test_winsound test_wmi test_zipfile64

Total duration: 31 min 25 sec

Click to see traceback logs
remote: Enumerating objects: 26, done.        
remote: Counting objects:   3% (1/26)        
remote: Counting objects:   7% (2/26)        
remote: Counting objects:  11% (3/26)        
remote: Counting objects:  15% (4/26)        
remote: Counting objects:  19% (5/26)        
remote: Counting objects:  23% (6/26)        
remote: Counting objects:  26% (7/26)        
remote: Counting objects:  30% (8/26)        
remote: Counting objects:  34% (9/26)        
remote: Counting objects:  38% (10/26)        
remote: Counting objects:  42% (11/26)        
remote: Counting objects:  46% (12/26)        
remote: Counting objects:  50% (13/26)        
remote: Counting objects:  53% (14/26)        
remote: Counting objects:  57% (15/26)        
remote: Counting objects:  61% (16/26)        
remote: Counting objects:  65% (17/26)        
remote: Counting objects:  69% (18/26)        
remote: Counting objects:  73% (19/26)        
remote: Counting objects:  76% (20/26)        
remote: Counting objects:  80% (21/26)        
remote: Counting objects:  84% (22/26)        
remote: Counting objects:  88% (23/26)        
remote: Counting objects:  92% (24/26)        
remote: Counting objects:  96% (25/26)        
remote: Counting objects: 100% (26/26)        
remote: Counting objects: 100% (26/26), done.        
remote: Compressing objects:   7% (1/14)        
remote: Compressing objects:  14% (2/14)        
remote: Compressing objects:  21% (3/14)        
remote: Compressing objects:  28% (4/14)        
remote: Compressing objects:  35% (5/14)        
remote: Compressing objects:  42% (6/14)        
remote: Compressing objects:  50% (7/14)        
remote: Compressing objects:  57% (8/14)        
remote: Compressing objects:  64% (9/14)        
remote: Compressing objects:  71% (10/14)        
remote: Compressing objects:  78% (11/14)        
remote: Compressing objects:  85% (12/14)        
remote: Compressing objects:  92% (13/14)        
remote: Compressing objects: 100% (14/14)        
remote: Compressing objects: 100% (14/14), done.        
remote: Total 14 (delta 12), reused 1 (delta 0), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '3847a6c64b96bb2cb93be394a590d4df2c35e876'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 3847a6c64b gh-92123: Convert `_elementtree` types to heap types (#99221)
Switched to and reset branch 'main'

Objects/obmalloc.c:776:1: warning: ‘always_inline’ function might not be inlinable [-Wattributes]
  776 | arena_map_get(pymem_block *p, int create)
      | ^~~~~~~~~~~~~

make: *** [Makefile:1911: buildbottest] Error 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants