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 — GH-126491: GC: Mark objects reachable from roots before doing cycle collection by markshannon · Pull Request #126502 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Nov 6, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This PR:

  • Performs a marking step before the incremental cycle collection
  • Rescans the stack before each increment
  • Removes lazy dict tracking as the lazy tracking optimization no longer pays off.

Performance is excellent. Speedup is 4%, with a 100% speedup on one GC benchmark.

Stats show the GC is run more frequently and collects more objects, but does only ~60% of the work.

This PR also:

  • Makes detaching an object's dict a bit more robust in case of a memory error.
  • Increases the threshold in a GC test. This test is to check for uncontrolled growth, so the exact threshold doesn't matter.
  • Issue: Mark all objects reachable from roots as live before doing main cyclic GC pass #126491

Sorry, something went wrong.

Uh oh!

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

efimov-mikhail, dolfinus, Fidget-Spinner, thinkwelltwd, methane, scastlara, iritkatriel, AlexWaygood, hugovk, bratao, and Object905 reacted with rocket emoji
All reactions
  • 🚀 11 reactions
markshannon added 8 commits November 4, 2024 11:42
@markshannon
GC experiment: mark almost all reachable objects before doing collect…
2ec8d8a
…ion phase
@markshannon
Add stats for objects marked
1fdf00e
@markshannon
Start with mark phase
5e813c5
@markshannon
Add stats for visits during marking
8bd7606
@markshannon
Visit new frames before each increment
3513da2
@markshannon
Redo stats
ab1faec
@markshannon
Fix freezing and GC untracking
9e2d93c
@markshannon
Don't untrack dicts
3c18fc8
@markshannon
markshannon requested review from ericsnowcurrently and methane as code owners November 6, 2024 14:23
@bedevere-app bedevere-app Bot added the awaiting core review label Nov 6, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Nov 6, 2024
Mark all objects reachable from roots as live before doing main cyclic GC pass #126491
Open
github-advanced-security[bot]
github-advanced-security AI found potential problems Nov 6, 2024
View reviewed changes
Comment thread Python/gc.c Fixed

Uh oh!

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

markshannon added 9 commits November 6, 2024 14:41
@markshannon
Remove lazy dict tracking from no-gil build
94da963
@markshannon
Remove unused variable
659fd1e
@markshannon
Add news
4cfbc4f
@markshannon
Fix use after free
8c92ca6
@markshannon
Attempt more careful fix of use-after-free
12d7f7c
@markshannon
Typo
1f619d7
@markshannon
Fix use of uninitialized variable
b55fe37
@markshannon
Fix compiler warnings
73b7f52
@markshannon
Tweak test
33f6386
iritkatriel
iritkatriel reviewed Nov 11, 2024
View reviewed changes
Comment thread Include/internal/pycore_object.h Resolved

Uh oh!

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

iritkatriel
iritkatriel requested changes Nov 11, 2024
View reviewed changes

@iritkatriel iritkatriel 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.

Could you update https://github.com/python/cpython/blob/main/InternalDocs/garbage_collector.md to explain how this works?

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot removed the awaiting core review label Nov 11, 2024
@bedevere-app

bedevere-app Bot commented Nov 11, 2024

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot added the awaiting changes label Nov 11, 2024
markshannon added 2 commits November 11, 2024 16:41
@markshannon
Add section to internal docs
8574d00
@markshannon
Rephrase new docs
70007b0
iritkatriel
iritkatriel reviewed Nov 11, 2024
View reviewed changes
Comment thread Include/internal/pycore_gc.h Resolved

Uh oh!

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

Comment thread Include/internal/pycore_runtime_init.h Resolved

Uh oh!

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

@markshannon
Make sure tuples are untracked and avoid quadratic time validation
278059b
AlexWaygood
AlexWaygood reviewed Nov 16, 2024
View reviewed changes
Comment thread InternalDocs/garbage_collector.md Resolved

Uh oh!

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

markshannon and others added 4 commits November 18, 2024 09:43
@markshannon @AlexWaygood
Update InternalDocs/garbage_collector.md
f186b4a
Co-authored-by: Alex Waygood <[email protected]>
@markshannon
Remove unused variable
5f6d04e
@markshannon
Tweak work to do calculation
9cfb5f0
@markshannon
Explain work to do calculation
c7683a4
iritkatriel
iritkatriel reviewed Nov 18, 2024
View reviewed changes
Comment thread Python/gc.c
}
}
if (!start && frame->visited) {
// If this frame has already been visited, then the lower frames

@iritkatriel iritkatriel Nov 18, 2024

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.

There's a warning here

Sorry, something went wrong.

Uh oh!

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

All reactions

@markshannon markshannon Nov 18, 2024

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.

It is a false positive. I think it is complaining because we don't set visited here, but this line is unreachable for entry frames.

I'll initialize visited to keep it quiet and in case the code gets reordered.

Sorry, something went wrong.

Uh oh!

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

All reactions
@markshannon
Initialize field to prevent code analyzer warning.
170ea6d
iritkatriel
iritkatriel approved these changes Nov 18, 2024
View reviewed changes
@markshannon
markshannon merged commit b0fcc2c into python:main Nov 18, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 18, 2024
@markshannon
markshannon deleted the mark-first-gc branch November 18, 2024 14:36
@AlexWaygood AlexWaygood mentioned this pull request Nov 18, 2024
Incremental GC causes a significant slowdown for Sphinx #124567
Closed
@hugovk

hugovk commented Nov 18, 2024

Copy link
Copy Markdown
Member

Hmm, looks like this commit b0fcc2c has caused test.test_gc.IncrementalGCTests.test_incremental_gc_handles_fast_cycle_creation failures for iOS and Android:

  • iOS ARM64 Simulator: https://buildbot.python.org/#/builders/1380/builds/1907
  • AMD64 Android: https://buildbot.python.org/#/builders/1591/builds/516
  • aarch64 Android: https://buildbot.python.org/#/builders/1594/builds/600

Reminder the 3.14 alpha 2 is due tomorrow (2024-11-19): https://buildbot.python.org/#/release_status

cc @freakboy3742 @mhsmith

freakboy3742 reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

@nascheme

nascheme commented Nov 18, 2024

Copy link
Copy Markdown
Member

Extracted from the Android failure:

FAIL: test_incremental_gc_handles_fast_cycle_creation (test.test_gc.IncrementalGCTests.test_incremental_gc_handles_fast_cycle_creation)
----------------------------------------------------------------------          
Traceback (most recent call last):                                              
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/contextlib.py", line 85, in inner
    return func(*args, **kwds)                                                  
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_gc.py", line 1175, in test_incremental_gc_handles_fast_cycle_creation 
    self.assertLess(new_objects, 25_000)                                        
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^                                        
AssertionError: 25032 not less than 25000                                       
                                                                                
----------------------------------------------------------------------          
Ran 53 tests in 3.744s                                                          
                                                                                
FAILED (failures=1, skipped=8)                                                  
test test_gc failed                                                             
test_gc failed (1 failure)                                                      
1 test failed again:                                                            
    test_gc         
All reactions

Sorry, something went wrong.

Uh oh!

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

@hugovk

hugovk commented Nov 18, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Also some tier 1 and 2 "test_import failed (reference leak)" that include this change (and some others) are now failing:

  • AMD64 RHEL8 Refleaks: https://buildbot.python.org/#/builders/259/builds/1712
  • aarch64 RHEL8 Refleaks: https://buildbot.python.org/#/builders/551/builds/348

Plus s390x RHEL8 + RHEL9 and AMD64 FreeBSD refleaks buildbots.

I'll open a draft revert PR so we can run the buildbots on it.

Edit: #126983

All reactions

Sorry, something went wrong.

Uh oh!

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

hugovk added a commit to hugovk/cpython that referenced this pull request Nov 18, 2024
@hugovk
Revert "pythonGH-126491: GC: Mark objects reachable from roots before…
2ac5c48
… doing cycle collection (pythonGH-126502)"

This reverts commit b0fcc2c.
@markshannon

markshannon commented Nov 18, 2024

Copy link
Copy Markdown
Member Author

Hmm, looks like this commit b0fcc2c has caused test.test_gc.IncrementalGCTests.test_incremental_gc_handles_fast_cycle_creation failures for iOS and Android:

I don't have ready access to an iOS or Android device to test on.
The failing test is designed to check that the heap doesn't keep growing. If the initial heap size is significantly smaller than on linux/windows then the heap would grow a bit more before stabilizing. Setting the limit to 27k or 30k should work.
It might be worth double checking that the heap doesn't continue to grow by increasing the number of iterations from 20k to 100k+

All reactions

Sorry, something went wrong.

Uh oh!

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

@freakboy3742

freakboy3742 commented Nov 18, 2024

Copy link
Copy Markdown
Contributor

I've done some local testing on iOS. The test_gc module passes as-is if you run it by itself. If I increase the loop count in the failing test to 100k, the test fails with exactly the same object count (25,032).

My guess is that this one of those issues that iOS/Android expose because they don't run the test suite in parallel - my guess is that you'd likely be able to reproduce this failure on a desktop machine if you run the test suite with --single-process.

On that basis, following @markshannon's suggestion I'll increase the object count to 26k. PR incoming.

All reactions

Sorry, something went wrong.

Uh oh!

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

@freakboy3742 freakboy3742 mentioned this pull request Nov 18, 2024
GH-126491: Increase the threshold for the GC fast cycle test. #126984
Closed
@freakboy3742

freakboy3742 commented Nov 18, 2024

Copy link
Copy Markdown
Contributor

#126984 is a PR increasing the test threshold.

All reactions

Sorry, something went wrong.

Uh oh!

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

@freakboy3742

freakboy3742 commented Nov 19, 2024

Copy link
Copy Markdown
Contributor

I've abandoned the "increase the threshold" aapproach - that's clearly not enough to avoid the issue. The threshold needed to pass the test is clearly much higher than 30k.

The good news (well... good for me, not so good for @markshannon 😄 ) is that I can reproduce this bug on macOS. If you run the test suite sequentially in a single process (./python.exe -m test -W --single-process), test_gc fails with the same error on macOS as is being reported on iOS and Android.

You don't see this error if you run test_gc in isolation - the 25k check passes. This is also consistent with the behavior on iOS and Android. You have to run the full sequence of (alphabetical) tests up to test_gc to see the failure. I'll take a quick pass to see if I can narrow down a smaller (and faster) reproduction case.

All reactions

Sorry, something went wrong.

Uh oh!

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

@freakboy3742

freakboy3742 commented Nov 19, 2024

Copy link
Copy Markdown
Contributor

I'm able to reliably reproduce the failure on macOS with ./python.exe -m test test_email test_fnmatch test_frame test_gc.

All reactions

Sorry, something went wrong.

Uh oh!

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

hugovk added a commit that referenced this pull request Nov 19, 2024
@hugovk
Revert "GH-126491: GC: Mark objects reachable from roots before doing…
899fdb2
… cycle collection (GH-126502)" (#126983)
@hugovk

hugovk commented Nov 19, 2024

Copy link
Copy Markdown
Member

I've reverted this because it's release day and we need to keep tier-1 green anyway: #126983.

Let's make sure to summon the buildbots next time :)

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

@markshannon markshannon mentioned this pull request Nov 19, 2024
Remove lazy dictionary tracking #127010
Closed
@nascheme

nascheme commented Nov 19, 2024

Copy link
Copy Markdown
Member

I'm able to re-produce on Linux with this set of tests: test_dis test_email test_funcattrs test_functools test_gc.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon mentioned this pull request Nov 21, 2024
GH-126491: GC: Mark objects reachable from roots before doing cycle collection #127110
Merged
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@markshannon @ebonnal
pythonGH-126491: GC: Mark objects reachable from roots before doing c…
452fbb4
…ycle collection (pythonGH-126502)

* Mark almost all reachable objects before doing collection phase

* Add stats for objects marked

* Visit new frames before each increment

* Remove lazy dict tracking

* Update docs

* Clearer calculation of work to do.
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@hugovk @ebonnal
Revert "pythonGH-126491: GC: Mark objects reachable from roots before…
53b8263
… doing cycle collection (pythonGH-126502)" (python#126983)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood left review comments
@iritkatriel iritkatriel iritkatriel approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@methane methane Awaiting requested review from methane methane is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

9 participants

@markshannon @ericsnowcurrently @hugovk @mdboom @iritkatriel @nascheme @freakboy3742 @github-advanced-security @AlexWaygood
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 01402520 — 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.