Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Let go of the event loop more often #1992

Merged
merged 8 commits into from
Aug 24, 2020
Merged

Conversation

carver
Copy link
Contributor

@carver carver commented Aug 20, 2020

What was wrong?

Related to #1980

How was it fixed?

  • Push some more long-running methods out of the event loop
  • Release the event loop in the middle of executing a series of methods

Bonuses:

  • Fix bug where set was modified while trie_fog was using it to mark some prefixes as complete
  • Improve the mid-import log during Beam Sync

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@carver carver requested a review from gsalgado August 20, 2020 23:21
current_batch += 1
if current_batch >= max_batch_size:
await asyncio.sleep(0)
current_batch = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you chose to release the event loop every 5 items instead of every single item? To me it'd make more sense to have iterators/generators produce as many items as they can if that doesn't require blocking, and then consumers can decide how to batch. However, @pipermerriam found that trio.ReceiveChannel releases on every iteration (ethereum/lahja#179 (comment)) and maybe we should be consistent with that? Either way, I'm not convinced this is the right place to implement the batching

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to honor the original intent there which seemed to be that it would be a bad idea to release it each time. I don't have a better reason than that, and it would certainly be simpler to just release each time. So I'll update it to release each cycle, if no one thinks it's important to be able to pump out batches of these.

carver added a commit to carver/trinity that referenced this pull request Aug 21, 2020
@carver
Copy link
Contributor Author

carver commented Aug 21, 2020

Anything else for a 👍 @gsalgado ?

@carver carver force-pushed the async-cooperation branch 2 times, most recently from 011d034 to dec2b48 Compare August 24, 2020 21:48
@carver carver merged commit 57bee06 into ethereum:master Aug 24, 2020
@carver carver deleted the async-cooperation branch August 25, 2020 18:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants