juspay / superposition

Superposition is a context-based configuration management system designed to facilitate safe and flexible rollout of config changes
https://juspay.io/open-source/superposition
Apache License 2.0
350 stars 14 forks source link

Request: Remove tokio & reqwest from client FFI library #183

Open ShreyBana opened 1 month ago

ShreyBana commented 1 month ago

Today, the FFI library is responsible for polling the server for updates. It uses tokio for scheduling an interval based chron & reqwest for performing HTTP requests. The problem with this is that it'll ship these dependencies to foreign language clients adding un-necessary weight to them. To get a tangible number I created a small binary w/ these dependencies, using minimal features & configured the build for size optimizations. Seems to be a little over 1MB. You can check it out here.
But size isn't the only issue, such dependencies can also limit platform support, as you can only ship to platforms supported by these libraries.
Removing tokio as a dependency shouldn't too hard, but reqwest might be a challenge. Maybe one can move to a lighter HTTP client like ureq & see if the size works out, but platform support might still be a challenge.
IMO, polling itself should just be moved out of the FFI library & into the foreign client.

Datron commented 1 month ago

Why we have tokio and can't remove it from the clients as they are today:

With regards to reqwest, this is the easier dependency to swap out if platform support can be handled.

What we can do:

This might cut down the size a bit. If this isn't enough, for platforms where size is a concern it may be better to write clients targeting that particular platform in the most optimal language for that platform. We can look into this as well.

ShreyBana commented 1 month ago

Tokio is the concurrency framework used by the rust clients where most of the functions are async (due to RwLock on the memory), the FFI segments need to block the threads since C doesn't support threading...

Doesn't the standard library provide an RW lock ? That way you could avoid dealing w/ any async business. Also, IDK if I'm understanding your statement correctly, but C does support threading. In-fact that's how the Linux kernel exposes it. I believe it's the clone() system call. Though you could also use some higher-level POSIX API.

.. It is exactly like this, the implementing language needs to provide the runtime for the FFI library...

I don't think this is accurate, from the code I can see that a runtime is being statically created inside the library itself. Also by asking to move it out, I meant that the implementation should be in the foreign language. That way you could keep the FFI library as light as possible.

... What platform doesn't tokio work on?

Wasn't really talking about tokio, my main concern was reqwest in that regard. tokio already has most of the platforms covered, the only issue is it's size.

With regards to reqwest, this is the easier dependency to swap out if platform support can be handled.

I would disagree here. Rust's standard library provides alternatives for the things you are using tokio for, so it'll be an easy change. On the other hand, finding a drop in for reqwest which solves the issues that I have mentioned seems hard.

Today we import tokio "full" feature set, we can reduce...

As I mentioned, to get an estimate on how much these 2 actually end up adding, I already did an experiment like this. There I am only using time for tokio & json for reqwest, plus some build flags to optimize for size. And even then it's a little over 1MB. Re-linking it here.

...If this isn't enough, for platforms where size is a concern it may be better to write clients targeting that particular platform in the most optimal language for that platform...

This is fine, but I still think that there's value in doing a FFI library with just the business logic. That will aid in sharing the code in such cases.

Datron commented 1 month ago

Doesn't the standard library provide an RW lock ? That way you could avoid dealing w/ any async business.

The standard library RwLock doesn't support async/await. It blocks the thread. Went with tokio since its runtime is more performant, supports async/await primitives and is more popular/well supported. Why not deal with async business? Tokio handles work stealing, thread scheduling and management.

Also, IDK if I'm understanding your statement correctly, but C does support threading. In-fact that's how the Linux kernel exposes it. I believe it's the clone() system call. Though you could also use some higher-level POSIX API.

Its not C that's supporting it, that is the underlying OS. For linux, you can use pthreads or fork() and exec(). It is different for windows. C does need OS level support to achieve this, and its different for different platforms. https://stackoverflow.com/questions/3908031/how-to-multithread-c-code

I don't think this is accurate, from the code I can see that a runtime is being statically created inside the library itself. Also by asking to move it out, I meant that the implementation should be in the foreign language. That way you could keep the FFI library as light as possible.

The tokio runtime that is initialized is mostly for spawning a blocking function or blocking a parallel operation to completion. If you don't run cac_start_polling_update inside a thread, it'll just block your main thread. For the other functions that call block_on, you don't need to thread and can just use it like a normal function. The implementing language can choose to make it async or not. Tokio maybe managing how much work is done here, we can take a look at a better way here.

I would disagree here. Rust's standard library provides alternatives for the things you are using tokio for, so it'll be an easy change. On the other hand, finding a drop in for reqwest which solves the issues that I have mentioned seems hard.

Sure, seems I was wrong in this regard. Though replacing tokio also is a challenge: https://users.rust-lang.org/t/lightweight-alternative-for-reqwest/33601/23

This is fine, but I still think that there's value in doing a FFI library with just the business logic. That will aid in sharing the code in such cases.

Agreed, but in some cases the constraints may justify another approach.

Datron commented 1 month ago

Maybe we can skip the discussions and just try to implement this?

A good way to check the feasibility is to try and remove tokio and reqwest and see if we can do that without comprising perf and safety. @ShreyBana would you be willing to contribute here?

knutties commented 1 month ago

That is a nice discussion. I think in general going granular makes sense and exposing the core CAC resolution logic as separate functions for FFI inclusion makes a lot of sense leaving the network implementation to the wrappers (while we can always supply a default network implementation wrapper) - @ShreyBana @Datron.

Before we jump into implementation - let us consider the CAC lang implication. I am thinking we should finalize the CAC lang specifics within about a week and that will have client implications as well.

Thanks for the detailed feedback - @ShreyBana