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 for ParkingLot and Event #1944

Closed
wants to merge 1 commit into from

Conversation

belm0
Copy link
Member

@belm0 belm0 commented Apr 5, 2021

Event instances can be very numerous in a Trio program, especially considering idioms like "create a new event on every set()". Event contains ParkingLot and neither were using slots, which is a source of garbage objects.

closes #1943

@belm0 belm0 requested a review from pquentin April 5, 2021 01:49
@codecov
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

Merging #1944 (ef3f94e) into master (6754c74) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head ef3f94e differs from pull request most recent head a21abe0. Consider uploading reports for the commit a21abe0 to get more accurate results

@@            Coverage Diff             @@
##           master    #1944      +/-   ##
==========================================
- Coverage   99.58%   99.50%   -0.09%     
==========================================
  Files         114      114              
  Lines       14617    14617              
  Branches     1116     1116              
==========================================
- Hits        14557    14544      -13     
- Misses         43       51       +8     
- Partials       17       22       +5     
Impacted Files Coverage Δ
trio/_core/_parking_lot.py 100.00% <100.00%> (ø)
trio/_sync.py 100.00% <100.00%> (ø)
trio/_core/tests/tutil.py 96.61% <0.00%> (-3.39%) ⬇️
trio/_core/_multierror.py 98.87% <0.00%> (-1.13%) ⬇️
trio/_core/tests/test_ki.py 99.13% <0.00%> (-0.87%) ⬇️
trio/_core/tests/test_multierror.py 99.27% <0.00%> (-0.73%) ⬇️
trio/tests/test_ssl.py 99.57% <0.00%> (-0.29%) ⬇️
trio/tests/test_testing.py 99.76% <0.00%> (-0.24%) ⬇️

@njsmith
Copy link
Member

njsmith commented Apr 5, 2021

LGTM. TBH we should probably rewrite Event at some point to use a plain set and wait_task_rescheduled, instead of all the heavyweight ParkingLot machinery. (I was very into ParkingLot when first writing trio's synchronization primitives, but my excitement has dropped over time.) If you're worried about Event's memory usage, do you want to do that here?

@belm0
Copy link
Member Author

belm0 commented Apr 5, 2021

merge blocked by #1938

@belm0
Copy link
Member Author

belm0 commented Apr 5, 2021

TBH we should probably rewrite Event at some point to use a plain set and wait_task_rescheduled, instead of all the heavyweight ParkingLot machinery.

The parts of ParkingLot used by Event() don't look heavy-- I guess Event could use a simple list of tasks and just iterate that on set(), vs. ParkingLot's OrderedDict and popitem() 🤷‍♂️

@njsmith
Copy link
Member

njsmith commented Apr 5, 2021 via email

belm0 added a commit to belm0/trio that referenced this pull request Apr 5, 2021
belm0 added a commit to belm0/trio that referenced this pull request Apr 6, 2021
njsmith added a commit that referenced this pull request Apr 7, 2021
…ngLot (#1948)

* implement Event directly with wait_task_rescheduled rather than ParkingLot

raised in #1944

* clear listening tasks set after rescheduling

Co-authored-by: Nathaniel J. Smith <njs@pobox.com>

* eliminate use of custom_sleep_data

* newsfragment

Co-authored-by: Nathaniel J. Smith <njs@pobox.com>
@belm0
Copy link
Member Author

belm0 commented Apr 7, 2021

closing in favor of #1948

@belm0 belm0 closed this Apr 7, 2021
@belm0 belm0 deleted the event_slots branch April 7, 2021 04:43
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.

use slots for ParkingLot and Event
3 participants