-
Notifications
You must be signed in to change notification settings - Fork 89
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
chore: reduce time spent in synchronous code #2627
Comments
MartinquaXD
added a commit
that referenced
this issue
Apr 18, 2024
# Description First task of #2627 Creating contract instances with `ethcontract-rs` is very costly (especially in tight loops). A very significant amount of time is spent creating instances to query allowances. # Changes Instead of creating 1 contract instance per token to fetch balances for we now create a single erc20 instance and reuse it for all `allowance()` calls. This is unfortunately pretty ugly since `ethcontract-rs` doesn't really expose the generated calldata very nicely and doesn't really compose with `web3`. ## How to test Correctness should be covered by e2e tests Performance was verified using the benchmark mentioned in #2627 It's a bit hard to see but fetching allowances went from ~20% to ~7.6% of the entire runtime. The overall runtime of the axum request handler went from ~74% -> 63% of the runtime. Overall I think the performance gains are significant enough to warrant the slightly ugly but very local performance hack until we replace `ethcontract-rs` with `alloy` entirely. Before <img width="1728" alt="Screenshot 2024-04-17 at 21 32 46" src="https://github.com/cowprotocol/services/assets/19190235/9d8d9cdf-ed72-4bff-a40b-c3cbf5418dbf"> Optimized <img width="1728" alt="Screenshot 2024-04-17 at 21 28 35" src="https://github.com/cowprotocol/services/assets/19190235/005aa2cd-7679-4d49-8220-c86cd25988a1">
This issue has been marked as stale because it has been inactive a while. Please update this issue or it will be automatically closed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Background
Since we are routing almost all the quotes through the driver we are seeing a few performance issues.
Currently my best guess is that we are spending a bunch of time in synchronous code which can clog the async runtime somewhat.
I built a very crude test setup which has all the major liquidity sources enabled and a colocated baseline solver as a quoter. Then I built a script that sends USDC -> USDT quote requests to the API with random sell amounts (to avoid caching).
I ran the script for 5 minutes and generated flamegraphs to show which optimizations could the most impactful.
I identified 3 major issues:
Creating contract instances using
ethcontract-rs
To call contract methods we currently have to instantiate contract instances. This is done with
Contract::at(web3, address)
and can be extremely costly. The reason is that although there is 0 dynamic data associate with correctly calling a contract methodethcontract-rs
relies on runtime values a lot. And creating those runtime values (contract instances) is very costly for contracts with many methods.Ideally we would replace
ethcontract-rs
with something likealloy
for calling contract methods in a type safe manor becausealloy
does not involve any runtime logic.Converting from old types to new types
In the driver we currently still have the
boundary
layer which converts legacy cold to types which fit nicely into our new domain layer. This conversion is pure overhead since eventually the old legacy code should not exist anymore. My tests showed that converting the uniswapv3 liquidity is the most costly so that is the one we should convert first.Serializing requests
Currently we send liquidity to solvers which rely on that. For auctions this is not a huge deal since auctions happen relatively rarely but quotes can happen 20 times per second so the serialization overhead hurts way more for those requests.
The most basic solution would probably be to introduce a new serialization format which is more performant but this obviously a huge breaking change so it would have to be opt-in but at least for the baseline solver it could be a huge gain.
This change obviously needs to be discussed extensively but could result in very significant performance improvements.
The text was updated successfully, but these errors were encountered: