pyapp-kit / psygnal

Python observer pattern (callback/event system). Modeled after Qt Signals & Slots (but independent of Qt)
https://psygnal.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
86 stars 15 forks source link

perf: let EventedSet use clear() method of underlying set #307

Closed DanGonite57 closed 6 months ago

DanGonite57 commented 6 months ago

Motivated by https://github.com/napari/napari/issues/6746, https://github.com/napari/napari/pull/6895

The default clear method implemented by collections.abc.MutableSet is very slow, which I guess is due to the fact that it is not backed by the C implementation of the builtin set.

I have attempted to implement a custom interface for clear, modelled on the existing one for discard. However, I don't entirely understand the intentions for the various hooks, BAILs, etc, so please do let me know if anything is inconsistent with the behaviours and styles of other methods.

bench.py

from psygnal.containers import EventedSet

num = 100_000

my_set = EventedSet(range(num))
my_set.events.items_changed.connect(print)
my_set.clear()

Before:

> hyperfine --warmup 1 .\bench.py
Benchmark 1: .\bench.py
  Time (mean ± σ):      4.257 s ±  0.050 s    [User: 3.640 s, System: 0.047 s]
  Range (min … max):    4.192 s …  4.345 s    10 runs

After:

> hyperfine --warmup 1 .\bench.py
Benchmark 1: .\bench.py
  Time (mean ± σ):     236.3 ms ±   3.4 ms    [User: 59.9 ms, System: 31.0 ms]
  Range (min … max):   231.6 ms … 243.9 ms    12 runs
codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (b0c2952) to head (c296570).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #307 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 22 22 Lines 2071 2078 +7 ========================================= + Hits 2071 2078 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codspeed-hq[bot] commented 6 months ago

CodSpeed Performance Report

Merging #307 will improve performances by 11.78%

Comparing DanGonite57:perf-clear (c296570) with main (b0c2952)

Summary

⚡ 1 improvements ✅ 65 untouched benchmarks

Benchmarks breakdown

Benchmark main DanGonite57:perf-clear Change
test_emit_time[lambda-2] 127.8 µs 114.3 µs +11.78%
tlambert03 commented 6 months ago

wow. thanks this is fantastic. LGTM

Sorry about the weird BAIL stuff... had to remind myself why it's there (clearly not obvious), I think it was mostly there for things like add where subclasses might want the ability to cancel the event, or to avoid an event entirely as you've done in the case if len(self) == 0. but your PR is perfect, thanks!

tlambert03 commented 6 months ago

btw, the codspeed benchmarks shown above don't capture this, but the asv benchmarks do and are awesome 🤩

Change Before [b0c29526]
After [3b7af9d8] Ratio Benchmark (Parameter)
- 1.15±0.01μs 689±7ns 0.6 benchmarks.EventedSetWithCallbackSuite.time_clear(10)
- 1.16±0.03μs 682±2ns 0.59 benchmarks.EventedSetSuite.time_clear(10)
- 4.83±0s 763±2ns 0 benchmarks.EventedSetSuite.time_clear(100000)
- 4.84±0.02s 794±7ns 0 benchmarks.EventedSetWithCallbackSuite.time_clear(100000)

"infinite speedup" :joy: