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

refactor(chain): expunge rocksdb from near-jsonrpc-primitives dependency tree #4651

Merged
merged 12 commits into from
Aug 20, 2021

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Aug 6, 2021

Under the initiative to build near-api-rs, we need to ensure its dependencies from within nearcore are as lightweight as possible. On initial inspection, near-jsonrpc-primitives, which should be lightweight enough to be used across the ecosystem, actually isn't, as it's dependency tree currently includes rocksdb. To remedy this, we extract useful structures from near-chunks and near-network into their own -primitives counterpart crates and depend on those instead.

New Crates

  • near-chunks-primitives from near-chunks
  • near-network-primitives from near-network

Analysis

Dump of cargo tree -p near-jsonrpc-primitives | wc -l:

   624 master (before this refactor)
   610 after introducing `near-chunks-primitives` (`rocksdb` still present)
   494 after introducing `near-network-primitives` (bye, bye, rocksdb)
   459 after refining `near-network{,-primitives}`
   457 after moving metrics back to `near-network` (thanks, @bowenwang1996)
   417 after removing `metric_recorder` and moving it's dependent back to `near-network` 

Test Plan

  • Ensure unit tests pass

An unfortunate consequence of this kind of code movearound is that git looses track of blame, but for reference, modules in either of the new crates that git thinks of as a "new file", was extracted from their original counterparts.

@miraclx miraclx changed the title refactor: expunge rocksdb from near-jsonrpc-primitives dependency tree refactor(chain): expunge rocksdb from near-jsonrpc-primitives dependency tree Aug 6, 2021
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/types.rs Outdated Show resolved Hide resolved
@frol
Copy link
Collaborator

frol commented Aug 9, 2021

This PR improved the compilation time and other metrics of near-cli-rs significantly! (see the before / after figures)

debug clean build time: 7m 44s / 2m 7s
debug incremental build time: 34s / 17s
debug binary size: 55.9 MB / 38.2 MB
dependencies: 442 / 390

@miraclx Great job!

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

What are the criteria for deciding what goes into network-primitives?

@@ -6,6 +6,8 @@ use near_metrics::{
use std::collections::HashMap;
use strum::VariantNames;

use lazy_static::lazy_static;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for moving this file to network-primitives? It feels to me that those counters should be defined in the network crate itself

Copy link
Contributor Author

@miraclx miraclx Aug 11, 2021

Choose a reason for hiding this comment

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

You're right. Nice catch!


What are the criteria for deciding what goes into network-primitives?

In this case, we just initialize the crate and move whatever's necessary to liberate near-client-primitives from the excess dependencies.
Going forward, we can incrementally move whatever's appropriate into their equivalent -primitives crates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the dependency on Edge come from? I don't think it is used outside of the network crate

Copy link
Contributor Author

@miraclx miraclx Aug 12, 2021

Choose a reason for hiding this comment

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

So, while near-client-primitives only required;

near_network::{recorder::MetricRecorder, types::{AccountOrPeerIdOrHash, KnownProducer}, PeerInfo}

In the case of near_network::types, I believed it was safe to extract as much as was possible since that module name typically encapsulates reusable primitives. And that worked fine with the only exception being Consolidate which is retained in near_network::types.

Following that logic, routing::Edge has a number of dependents in near_network_primitives::types. So, naturally, it's extracted alongside it.


That aside, recorder::MetricRecorder depends on types::PeerMessage which depends on types::Edge

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believed it was safe to extract as much as was possible since that module name typically encapsulates reusable primitives

Well that is exactly my concern. Edge should not be used outside of the network crate. Why do we need MetricRecorder in near-client-primitives?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. @miraclx please remove metric_recorder from NetworkInfoResponse

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mfornet @bowenwang1996 should the whole feature be removed altogether or just drop the field from the NetworkInfoResponse? Is the feature still useful without the exposed data?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mfornet @bowenwang1996 should the whole feature be removed altogether or just drop the field from the NetworkInfoResponse? Is the feature still useful without the exposed data?

I think the feature should be removed altogether, but my point is that we can now revert the change that extracts network primitives since I don't think they should be in a separate crate to begin with @miraclx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MetricRecorder was removed, which freed up the dependency on PeerMessage which, in-turn, freed up the dependency on Edge. This means ::routing and a number of structures in ::types we're returned to near-network.
But we (near_client_primitives) still need near_network_primitives::types::{AccountOrPeerIdOrHash, KnownProducer, PeerInfo}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bowenwang1996 @mfornet Could you review the PR after the update?

@miraclx miraclx force-pushed the sherlock/deps/jsonrpc-primitives branch from 9a4d37c to e76b1a2 Compare August 11, 2021 14:44
Comment on lines 8 to 9
#[cfg(feature = "metric_recorder")]
pub use near_network_primitives::recorder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to also need to be cleaned up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe we want to keep the feature in the Cargo.toml file since there are other places where metric_recorder feature is used

@miraclx miraclx force-pushed the sherlock/deps/jsonrpc-primitives branch from 54a36c5 to 6eb6206 Compare August 18, 2021 17:21
@near-bulldozer near-bulldozer bot merged commit e242f71 into near:master Aug 20, 2021
@miraclx miraclx deleted the sherlock/deps/jsonrpc-primitives branch August 20, 2021 02:43
near-bulldozer bot pushed a commit that referenced this pull request Oct 4, 2021
…te (#4912)

#4651 successfully removed `near-jsonrpc-primitives`-s heavy dependency on `near-network` which in turn depended on `rocksdb`. #4112 reintroduced that dependency on `near-network`, though optionally, this hinders the publishing process.

This PR extracts the adversarial features that depend on `near-network` into a private, specialized crate: `near-jsonrpc-adversarial-primitives` and sets us right on track for publishing `jsonrpc-primitives`.
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.

5 participants