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

Use __slots__ in Partition #363

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Use __slots__ in Partition #363

merged 1 commit into from
Jan 5, 2022

Conversation

pjrule
Copy link
Contributor

@pjrule pjrule commented Aug 30, 2021

(I'm not sure if this is worth it yet, but figured I'd throw up a PR and we can do some benchmarks.) I was just reminded that using __slots__ is known to improve memory usage (and sometimes performance) of Python classes (see here), at least when the classes are small. I suspect our memory usage is dominated by large assignment dictionaries, but maybe this is a step in the right direction. __slots__ also protects against abuse of the Partition class by preventing the use of additional fields, though this may break some people's code.

@InnovativeInventor
Copy link
Member

@pjrule I'm going to give this a rebase and run some performance benchmarks. If we see a sizable improvement in replay speeds, I think it'd be worth merging in.

@InnovativeInventor
Copy link
Member

With the benchmarking utils I wrote a while back to look for performance regressions, I can confirm that this simple change results in a massive speedup in replay times. I estimate this gives a ~2-3x speedup in replay performance.

Replay performance without __slots__:

10 loops, best of 5: 1.42 sec per loop

Same chain replay performance with __slots__:

10 loops, best of 5: 610 msec per loop

We should totally merge this in!

Copy link
Member

@InnovativeInventor InnovativeInventor left a comment

Choose a reason for hiding this comment

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

Excellent speedup in replay times!

@InnovativeInventor
Copy link
Member

With the benchmarking utils I wrote a while back to look for performance regressions, I can confirm that this simple change results in a massive speedup in replay times. I estimate this gives a ~2-3x speedup in replay performance.

So . . . turns out this is not true. I was benchmarking off an old commit in main (before #367 was merged in), so the amended benchmarks on 10k replays are as follows:

  • 194 it/s (Parker's slots)
  • 189 it/s (main branch)

It's still worth it, though (every little bit counts).

@InnovativeInventor InnovativeInventor force-pushed the slots branch 2 times, most recently from 99f6904 to 5489c20 Compare January 5, 2022 22:08
@InnovativeInventor
Copy link
Member

Note that once we merge this in, our next release should be a major one (we should follow semver).

@InnovativeInventor InnovativeInventor merged commit f9f73a5 into main Jan 5, 2022
@InnovativeInventor InnovativeInventor mentioned this pull request Jan 6, 2022
@InnovativeInventor InnovativeInventor deleted the slots branch May 24, 2022 16:07
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