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

Non-blocking renderers #1422

Merged
merged 5 commits into from
Aug 5, 2021
Merged

Non-blocking renderers #1422

merged 5 commits into from
Aug 5, 2021

Conversation

dahlia
Copy link
Contributor

@dahlia dahlia commented Aug 3, 2021

This implements #1402.

Currently BlockChain<T> and Swarm<T> wait for registered renderers to get their jobs done for every render event. These render calls can be nonblocking if there is a renderer decorator which maintains its own background thread and a queue/channel to communicate between the main thread and the background thread.


Relevant pull requests on Nine Chronicles:

@dahlia dahlia self-assigned this Aug 3, 2021
@auto-assign auto-assign bot requested a review from riemannulus August 3, 2021 08:54
@dahlia dahlia linked an issue Aug 3, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #1422 (018f5a7) into main (51c8b15) will decrease coverage by 0.03%.
The diff coverage is 73.11%.

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

@@            Coverage Diff             @@
##             main    #1422      +/-   ##
==========================================
- Coverage   77.30%   77.27%   -0.04%     
==========================================
  Files         256      258       +2     
  Lines       17225    17318      +93     
==========================================
+ Hits        13316    13382      +66     
- Misses       3358     3380      +22     
- Partials      551      556       +5     
Impacted Files Coverage Δ
...anet/Blockchain/Renderers/DelayedActionRenderer.cs 82.40% <ø> (ø)
...net/Blockchain/Renderers/NonblockActionRenderer.cs 71.42% <71.42%> (ø)
Libplanet/Blockchain/Renderers/NonblockRenderer.cs 73.41% <73.41%> (ø)
Libplanet/Store/DefaultStore.cs 87.21% <0.00%> (-1.24%) ⬇️
Libplanet/Net/Protocols/KademliaProtocol.cs 67.41% <0.00%> (-0.38%) ⬇️
Libplanet/Net/Swarm.cs 84.58% <0.00%> (+0.08%) ⬆️
Libplanet.RocksDBStore/RocksDBStore.cs 78.44% <0.00%> (+0.25%) ⬆️
Libplanet/Net/BlockCompletion.cs 95.07% <0.00%> (+0.86%) ⬆️

limebell
limebell previously approved these changes Aug 3, 2021
Libplanet/Blockchain/Renderers/NonblockRenderer.cs Outdated Show resolved Hide resolved
/// Queues the callback to be executed in the worker thread.
/// </summary>
/// <param name="action">The callback to be executed in the worker thread.</param>
protected void Queue(System.Action action)
Copy link
Member

Choose a reason for hiding this comment

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

It also looks similar toTaskScheduler.QueueTask. It would have been nice to be able to use an implementation like SingleThreadedTaskScheduler, but there is no implementation in advance. 😢

Libplanet/Blockchain/Renderers/NonblockRenderer.cs Outdated Show resolved Hide resolved
@dahlia
Copy link
Contributor Author

dahlia commented Aug 5, 2021

@planetarium/libplanet Addressed requested changes. Please review this again!

longfin
longfin previously approved these changes Aug 5, 2021
limebell
limebell previously approved these changes Aug 5, 2021
@dahlia dahlia dismissed stale reviews from limebell and longfin via 5685b13 August 5, 2021 08:38
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.

NonblockRenderer<T> & NonblockActionRenderer<T>
3 participants