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

Explicit selection of RPCs using cargo features #29

Closed
wants to merge 6 commits into from
Closed

Explicit selection of RPCs using cargo features #29

wants to merge 6 commits into from

Conversation

okjodom
Copy link

@okjodom okjodom commented Dec 14, 2022

Allow explicit selection of RPCs using caargo features, for slimmer dependency and faster compilation. If no feature is selected, all are included by default

  1. Makes Lightning RPC optionally available under lightningrpc feature gate.

Usage: cargo run --features=lightningrpc --example getinfo <address> <tls.cert> <file.macaroon>

  1. Makes Wallet RPC optionally available under walletrpc feature gate.

Usage: cargo run --features=walletrpc --example <example> <address> <tls.cert> <file.macaroon>

  1. Makes Version RPC optionally available under versionrpc feature gate.

Usage: cargo run --features=versionrpc --example getversion <address> <tls.cert> <file.macaroon>

  1. Make Sign and Peers RPCs available under signrpc and peersrpc feature gates respectively

closes #26

@Kixunil
Copy link
Owner

Kixunil commented Dec 14, 2022

Why is it behind feature gate and not unconditional? Does it significantly increase compile time?

@okjodom
Copy link
Author

okjodom commented Dec 14, 2022

Why is it behind feature gate and not unconditional? Does it significantly increase compile time?

I haven't benchmarked compile times. I'm using the feature gates to allow very selective dependency on the RPCs. Specifically, I have a use case that needs LND routerrpc and never any of the other rpcs that can be supported

@Kixunil
Copy link
Owner

Kixunil commented Dec 14, 2022

Would be cool to benchmark it a bit. If it doesn't increase compile time by let's say more than 5 seconds on some reasonable machine I probably wouldn't bother much with gates. But I don't mind them too much either.

@okjodom
Copy link
Author

okjodom commented Dec 14, 2022

Would be cool to benchmark it a bit. If it doesn't increase compile time by let's say more than 5 seconds on some reasonable machine I probably wouldn't bother much with gates. But I don't mind them too much either.

Ack. Posting benchmarks below. I still would want gates so I I can pick this feature and shake out the rest of the features. If routerrpc were unconditional, and we end up with some other unconditional feature, that also came along for the ride, I would be keen on avoiding the bloat.

PS: We could pick what features to make unconditional by observing what features are depended on most often by other features in the feature table

/* Cargo.toml */

[features]
lightningrpc = []
signrpc = []
walletrpc = ["signrpc"]
routerrpc = ["lightningrpc"]
all = ["lightningrpc", "walletrpc", "routerrpc"]

I suspect lightningrpc might end up being such a feature

@okjodom
Copy link
Author

okjodom commented Dec 15, 2022

Would be cool to benchmark it a bit. If it doesn't increase compile time by let's say more than 5 seconds on some reasonable machine I probably wouldn't bother much with gates. But I don't mind them too much either.

Ack. Posting benchmarks below.

Effect of feature gates on compile times.
Benchmark compilation for `track_payment` example.

1. All features on by default : No feature gates applied

Fresh compile: 9.49s

                       | dev [unoptimized + debuginfo] target(s) |    AVG
- Clean compile        |    2.86s, 2.66s, 2.63s, 2.61s, 2.80s    |  ~ 2.71s
- Recompile (baseline) |    0.06s, 0.08s, 0.08s, 0.08s, 0.07s    |  ~ 0.07s


2. All features on by default : Feature gate `all` applied on compile

Fresh compile: 4.67s

                       | dev [unoptimized + debuginfo] target(s) |    AVG
- Clean compile        |    2.65s, 2.61s, 2.60s, 2.83s, 2.57s    |  ~ 2.65s
- Recompile (baseline) |    0.08s, 0.07s, 0.06s, 0.08s, 0.06s    |    0.07s


3. All features off by default : Only `routerrpc` feature is applied on compile 

Fresh compile: 4.13s

                       | dev [unoptimized + debuginfo] target(s) |    AVG
- Clean compile        |    2.51s, 2.55s, 2.66s, 2.52s, 2.52s    |  ~ 2.55s
- Recompile (baseline) |    0.07s, 0.08s, 0.07s, 0.08s, 0.08s    |  ~ 0.08s

------------------------------------------------------------------------------

Notes:

Fresh compile - Do a compilation after making a code change, for example, after deleting / reintroducing feature gate markers
Clean compile - Do a compilation after deleting the assets produced in target dir, but without code changes
Re compile    - Do a compilation immediately after clean compile. There are assets cached in the target dir, and is no code change to compile

Benchmarks on:
- 32 GiB RAM
- Intel® Core™ i7-9750H CPU @ 2.60GHz × 12 Processor
- Ubuntu 22.04.1
- rustc 1.65.0 (897e37553 2022-11-02)

@Kixunil , I collected the above benchmarks.
IMO, this looks much in favour of feature gates, especially as more features are added to support the full LND RPC API

@Kixunil
Copy link
Owner

Kixunil commented Dec 15, 2022

Cool, thank! It's funny that it really is those 5 sec. But the relative factor is ~2 which is a lot. I agree with having the gates.

@okjodom okjodom changed the title Optional Router RPC under feature gate Explicit selection of RPCs using cargo features Jun 27, 2023
@okjodom okjodom marked this pull request as ready for review June 27, 2023 21:02
@okjodom
Copy link
Author

okjodom commented Jun 27, 2023

@Kixunil, I aggregated a bunch of my long running contributions under this PR. Primarily, it adds feature gates and adds Router RPC.

Figured I could make all features available by default to avoid a breaking change for existing dependents in the wild. Dependents can now opt-out of features by applying explicit selections of rpcs they need

@okjodom
Copy link
Author

okjodom commented Oct 23, 2023

These changes are included in fedimint fork of this library. Closing PR as stale.

@okjodom okjodom closed this Oct 23, 2023
@okjodom okjodom deleted the routerrpc-feature branch October 23, 2023 22:42
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.

Optional RPCs support using features
2 participants