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-118013: Use weakrefs for the cache key in `inspect._shadowed_dict` by AlexWaygood · Pull Request #118202 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Apr 23, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Slower than main, but significantly faster than a straight revert of #104267.

  • Issue: [3.12] getattr_static can result in reference leaks #118013

Sorry, something went wrong.

Uh oh!

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

All reactions
AlexWaygood added 4 commits April 23, 2024 23:36
@AlexWaygood
wip
c314217
@AlexWaygood
wip
e4b2989
@AlexWaygood
wip
57da345
@AlexWaygood
wip
4df5d08
@bedevere-app bedevere-app Bot mentioned this pull request Apr 23, 2024
[3.12] getattr_static can result in reference leaks #118013
Closed
@AlexWaygood
Merge branch 'main' into inspect-weakref-cache-key
fd9b22b
@AlexWaygood
AlexWaygood requested a review from carljm April 23, 2024 23:12
carljm
carljm reviewed Apr 24, 2024
View reviewed changes

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

Looks good to me, once we get news/tests/benchmarks.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/inspect.py Resolved

Uh oh!

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

AlexWaygood added 3 commits April 24, 2024 12:07
@AlexWaygood
Tests and comments
5847027
@AlexWaygood
Clear the cache anyway after running test_inspect, for extra safety
677577e
@AlexWaygood
docs
6891c70
@AlexWaygood
AlexWaygood marked this pull request as ready for review April 24, 2024 11:23
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 24, 2024
@AlexWaygood
Merge branch 'main' into inspect-weakref-cache-key
4791b4b
@AlexWaygood

AlexWaygood commented Apr 24, 2024

Copy link
Copy Markdown
Member Author

Benchmarks

All timings done with a fresh PGO-optimised build on main using a MacBook pro.

getattr_static benchmarks:

Benchmark script
from timeit import timeit
from statistics import mean, stdev
from inspect import getattr_static

class Foo:
    @property
    def x(self) -> int:
        return 42

class Bar:
    x = 42

class WithParentClassX(Bar): ...

class Baz:
    def __init__(self):
        self.x = 42

class WithParentX(Baz): ...

class Missing: ...

class Slotted:
    __slots__ = ('x',)
    def __init__(self):
        self.x = 42

class Method:
    def x(self): ...

class ClsMethod:
    @classmethod
    def x(cls): ...

class StMethod:
    @staticmethod
    def x(): ...

import gc
gc.disable()

times = []
def stats():
    ts = [t * 1e8 for t in sorted(times)[:5]]
    return f'{round(mean(ts)):4} ± {round(stdev(ts)):2} ns '

def bench(obj):
    # Warmup:
    for _ in range(5):
        number = 100
        timeit(lambda: getattr_static(obj, 'x', None), number=number)

    # Actual bench:
    for _ in range(50):
        number = 1000
        t = timeit(lambda: getattr_static(obj, 'x', None), number=number) / number
        times.append(t)

    bench_name = (
        f'type[{obj.__name__}]'
        if isinstance(obj, type)
        else obj.__class__.__name__
    )
    print(f"{bench_name: <25}: {stats()}")
    times.clear()


bench(Foo)
bench(Foo())
bench(Bar)
bench(Bar())
bench(WithParentClassX())
bench(Baz())
bench(WithParentX())
bench(Missing)
bench(Missing())
bench(Slotted())
bench(Method())
bench(StMethod())
bench(ClsMethod())
Results on main
type[Foo]                :   26 ±  1 ns 
Foo                      :   44 ±  0 ns 
type[Bar]                :   26 ±  0 ns 
Bar                      :   40 ±  0 ns 
WithParentClassX         :   52 ±  0 ns 
Baz                      :   49 ±  0 ns 
WithParentX              :   59 ±  0 ns 
type[Missing]            :   56 ±  0 ns 
Missing                  :   50 ±  0 ns 
Slotted                  :   57 ±  0 ns 
Method                   :   39 ±  0 ns 
StMethod                 :   39 ±  0 ns 
ClsMethod                :   39 ±  0 ns
Results with this PR
type[Foo]                :   39 ±  0 ns 
Foo                      :   60 ±  1 ns 
type[Bar]                :   33 ±  0 ns 
Bar                      :   56 ±  0 ns 
WithParentClassX         :   79 ±  0 ns 
Baz                      :   72 ±  0 ns 
WithParentX              :   92 ±  0 ns 
type[Missing]            :   86 ±  0 ns 
Missing                  :   74 ±  0 ns 
Slotted                  :   74 ±  0 ns 
Method                   :   56 ±  0 ns 
StMethod                 :   56 ±  0 ns 
ClsMethod                :   56 ±  0 ns
Results with a clean revert of #104267
type[Foo]                :   43 ±  0 ns 
Foo                      :   66 ±  1 ns 
type[Bar]                :   37 ±  0 ns 
Bar                      :   65 ±  0 ns 
WithParentClassX         :   91 ±  0 ns 
Baz                      :   84 ±  0 ns 
WithParentX              :  111 ±  0 ns 
type[Missing]            :  103 ±  0 ns 
Missing                  :   85 ±  0 ns 
Slotted                  :   78 ±  0 ns 
Method                   :   65 ±  0 ns 
StMethod                 :   65 ±  0 ns 
ClsMethod                :   65 ±  0 ns

Runtime-checkable protocol benchmarks

Benchmark script
from typing import Protocol, runtime_checkable

@runtime_checkable
class Foo(Protocol):
    a: int
    b: int
    c: int
    d: int
    e: int
    f: int
    g: int
    h: int
    i: int
    j: int
    k: int
    l: int
    m: int
    n: int
    o: int
    p: int
    q: int
    r: int
    s: int
    t: int
    u: int
    v: int
    w: int
    x: int
    y: int
    z: int

class Bar:
    def __init__(self):
        for attrname in 'abcdefghijklmnopqrstuvwxyz':
            setattr(self, attrname, 42)

bars = [Bar() for _ in range(100_000)]

import time
start = time.perf_counter()
for bar in bars:
    isinstance(bar, Foo)
print(time.perf_counter() - start)

Results on main: 1.3446190829854459

Results with this PR: 1.9680303339846432

Results with a clean revert of #104267: 2.243281374918297

All reactions

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood
AlexWaygood requested a review from carljm April 24, 2024 11:39
@AlexWaygood AlexWaygood added the needs backport to 3.12 only security fixes label Apr 24, 2024
@AlexWaygood

AlexWaygood commented Apr 24, 2024

Copy link
Copy Markdown
Member Author

(The getattr_static() benchmark script was originally contributed by @sobolevn in #103193 (comment))

All reactions

Sorry, something went wrong.

Uh oh!

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

sobolevn
sobolevn reviewed Apr 24, 2024
View reviewed changes
Comment thread Lib/inspect.py Resolved

Uh oh!

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

carljm
carljm approved these changes Apr 24, 2024
View reviewed changes

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

🎉

Sorry, something went wrong.

Uh oh!

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

AlexWaygood reacted with rocket emoji
All reactions
  • 🚀 1 reaction
Comment thread Lib/test/test_inspect/test_inspect.py Resolved

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 24, 2024
@AlexWaygood
AlexWaygood merged commit 8227883 into python:main Apr 24, 2024
@AlexWaygood
AlexWaygood deleted the inspect-weakref-cache-key branch April 24, 2024 14:55
@miss-islington-app

miss-islington-app Bot commented Apr 24, 2024

Copy link
Copy Markdown

Thanks @AlexWaygood for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 24, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 24, 2024
@AlexWaygood @miss-islington
pythongh-118013: Use weakrefs for the cache key in `inspect._shadowed…
fa9d669
…_dict` (pythonGH-118202)

(cherry picked from commit 8227883)

Co-authored-by: Alex Waygood <[email protected]>
@bedevere-app

bedevere-app Bot commented Apr 24, 2024

Copy link
Copy Markdown

GH-118232 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Apr 24, 2024
AlexWaygood added a commit that referenced this pull request Apr 24, 2024
@miss-islington @AlexWaygood
[3.12] gh-118013: Use weakrefs for the cache key in `inspect._shadowe…
0691bd8
…d_dict` (GH-118202) (#118232)

Co-authored-by: Alex Waygood <[email protected]>
@eendebakpt eendebakpt mentioned this pull request Apr 30, 2025
Understand the outlier benchmarks on 3.14 (main) vs. 3.13.0 faster-cpython/ideas#726
Open
5 of 25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevn sobolevn sobolevn left review comments
@carljm carljm carljm approved these changes

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.

3 participants

@AlexWaygood @carljm @sobolevn
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.