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.

Sign up for GitHub

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

Jump to bottom

Introduce and use Sink abstraction #9962

Merged
merged 4 commits into from
Mar 27, 2022
Merged

Introduce and use Sink abstraction #9962

merged 4 commits into from
Mar 27, 2022

Conversation

georgeee
Copy link
Member

Problem: there are a few layers of pipes that are used between sites of
pubsub message receival and pubsub message processing. These layers
complicate reasoning about code and impose an unnecessary additional load
onto Async scheduler.

Solution: introduce a Sink abstraction and use it to handle all gossips.

Explain your changes:

  • Introduce sink abstraction
  • Remove all intermediate pipes for gossip message processing

Explain how you tested your changes:

  • No change to behavior was made
  • Existing tests pass

Checklist:

  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? None

@georgeee georgeee requested review from a team as code owners December 23, 2021 16:07
@georgeee georgeee added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Dec 23, 2021
georgeee
georgeee commented Dec 24, 2021
View reviewed changes
src/lib/gossip_net/libp2p.ml Outdated Show resolved Hide resolved
src/lib/cli_lib/default.ml Outdated Show resolved Hide resolved
src/lib/mina_networking/mina_networking.ml Outdated
}) ;
Mina_net2.Validation_callback.set_message_type valid_cb `Block ;
Mina_metrics.(Counter.inc_one Network.Block.received) ;
notify_online ())
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it fine to run notify_online without don't_wait_for?

Can it produce a stale-lock?

Copy link
Member

Choose a reason for hiding this comment

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

The pipe being written to under the hood of Broadcast_pipe is an Async.Pipe which is consumed with pushback. This means that the write call will block until a reader picks up the value and finishes processing it. I would just put a don't_wait_for here to be safe because it's possible this could produce a stale lock in some conditions.

src/lib/mina_networking/sinks.ml Outdated Show resolved Hide resolved
src/lib/network_peer/envelope.ml Outdated Show resolved Hide resolved
src/lib/network_pool/pool_sink.ml Outdated Show resolved Hide resolved
src/lib/network_pool/pool_sink.ml Outdated Show resolved Hide resolved
@georgeee georgeee force-pushed the georgeee/sink-refactoring branch from 7eee16c to f175181 Compare December 24, 2021 13:07
@georgeee georgeee force-pushed the georgeee/sink-refactoring branch 3 times, most recently from a4a61bf to 0379e2f Compare December 24, 2021 20:30
@georgeee georgeee force-pushed the georgeee/sink-refactoring branch from 0379e2f to 051daba Compare January 7, 2022 11:44
nholland94
nholland94 requested changes Jan 7, 2022
View reviewed changes
src/lib/fake_network/fake_network.ml Show resolved Hide resolved
src/lib/gossip_net/any.ml Outdated Show resolved Hide resolved
src/lib/gossip_net/libp2p.ml Outdated Show resolved Hide resolved
src/lib/mina_networking/mina_networking.ml Outdated
}) ;
Mina_net2.Validation_callback.set_message_type valid_cb `Block ;
Mina_metrics.(Counter.inc_one Network.Block.received) ;
notify_online ())
Copy link
Member

Choose a reason for hiding this comment

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

The pipe being written to under the hood of Broadcast_pipe is an Async.Pipe which is consumed with pushback. This means that the write call will block until a reader picks up the value and finishes processing it. I would just put a don't_wait_for here to be safe because it's possible this could produce a stale lock in some conditions.

src/lib/mina_networking/mina_networking.ml Outdated Show resolved Hide resolved
src/lib/mina_intf/transition_frontier_components_intf.ml Outdated Show resolved Hide resolved
src/lib/network_pool/pool_sink.ml Outdated Show resolved Hide resolved
src/lib/network_pool/pool_sink.ml Outdated
let cb' = Msg.convert_callback cb in
match%bind verify_impl ~logger ~trace_label pool rl env' cb' with
| None ->
(* TODO log unverified? *)
Copy link
Member

Choose a reason for hiding this comment

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

This TODO needs to be resolved.

src/lib/network_pool/pool_sink.ml Outdated Show resolved Hide resolved
@georgeee georgeee force-pushed the georgeee/sink-refactoring branch 2 times, most recently from 243a1f4 to 74c7103 Compare March 17, 2022 23:11
nholland94
nholland94 approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@nholland94 nholland94 left a comment

Choose a reason for hiding this comment

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

LGTM. My last piece of feedback is that I'm not sure that block_sink.ml lives in a logical location right now (it's currently in the network_pool library, which currently contains snark and txn pool code, but not block code). Perhaps mina_transition (where blocks are defined) or transition_handler (where incoming blocks are processed) makes more sense.

@georgeee georgeee force-pushed the georgeee/sink-refactoring branch from f7fdd16 to de25253 Compare March 23, 2022 23:00
@georgeee
Introduce and use Sink abstraction
b8a521c 
Problem: there are a few layers of pipes that are used between sites of
pubsub message receival and pubsub message processing. These layers
complicate reasoning about code and impose an unnecessary additional load
onto Async scheduler.

Solution: introduce a Sink abstraction and use it to handle all gossips.
@georgeee georgeee force-pushed the georgeee/sink-refactoring branch from de25253 to b8a521c Compare March 24, 2022 23:09
@georgeee georgeee force-pushed the georgeee/sink-refactoring branch from aaf2dc5 to b227493 Compare March 25, 2022 15:31
georgeee added 2 commits March 25, 2022 22:45
@georgeee
Fix O1trace.sync_thread names
2c5579c
@georgeee
Update metrics for snark and tx pools
42843d7 
Problem: during refactoring, tx pool and snark pool metrics were not
updated properly.

Solution: update metrics as needed.
@georgeee georgeee force-pushed the georgeee/sink-refactoring branch from 5a5bd5e to 42843d7 Compare March 27, 2022 18:26
@georgeee georgeee merged commit d9bf5e7 into compatible Mar 27, 2022
@georgeee georgeee deleted the georgeee/sink-refactoring branch March 27, 2022 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nholland94 nholland94 nholland94 approved these changes

@bkase bkase Awaiting requested review from bkase

@imeckler imeckler Awaiting requested review from imeckler

@mrmr1993 mrmr1993 Awaiting requested review from mrmr1993

@psteckler psteckler Awaiting requested review from psteckler

Assignees
No one assigned
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch proj-network-stability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@georgeee @nholland94