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

Make Pieces immutable and return them from cache #921

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akx
Copy link

@akx akx commented Sep 23, 2022

This PR makes Piece a frozen dataclass and adds a cache for them. For end users (and the library itself of course) this change is invisible API-wise.

However, there is a visible performance increase (even if the test suite is not necessarily the best indicator):

hyperfine 'git checkout master; git clean -fdx; py.test --count=10 test.py' 'git checkout immutable-pieces; git clean -fdx; py.test --count=10 test.py'
Benchmark 1: git checkout master; git clean -fdx; py.test --count=10 test.py
  Time (mean ± σ):      6.405 s ±  0.411 s    [User: 5.249 s, System: 0.404 s]
  Range (min … max):    6.120 s …  7.537 s    10 runs

Benchmark 2: git checkout immutable-pieces; git clean -fdx; py.test --count=10 test.py
  Time (mean ± σ):      6.246 s ±  0.131 s    [User: 5.218 s, System: 0.394 s]
  Range (min … max):    6.126 s …  6.591 s    10 runs

Summary
  'git checkout immutable-pieces; git clean -fdx; py.test --count=10 test.py' ran
    1.03 ± 0.07 times faster than 'git checkout master; git clean -fdx; py.test --count=10 test.py'

This will also likely have a nice memory usage effect.

Let me know what you think :)

@niklasf
Copy link
Owner

niklasf commented Sep 25, 2022

Hi, thanks! I like this patch.

Even as is, pieces are intended to be immutable (no setters, provides __hash__). But that's not documented, so this is technically still a bc break and needs to wait for a future version 2.x.

@niklasf niklasf added this to the v2.0.0 milestone Jul 10, 2023
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.

2 participants