Skip to content
This repository was archived by the owner on Mar 14, 2025. It is now read-only.

Cache RMN Contract in OnRamp#945

Merged
asoliman92 merged 9 commits intoccip-developfrom
chore/CCIP-2391-cache-arm-contract
Jun 3, 2024
Merged

Cache RMN Contract in OnRamp#945
asoliman92 merged 9 commits intoccip-developfrom
chore/CCIP-2391-cache-arm-contract

Conversation

@asoliman92
Copy link
Contributor

@asoliman92 asoliman92 commented May 31, 2024

Motivation

Every call to arm_contract.NewARMContract deserialized ARM contract abi under the hood. Considering that OnRamp.IsSourceCursed is called very often by the background worker it creates a huge CPU overhead. Please initialize ARMContract only once and cache it instead of doing it with every method call

Solution

  • Add new cachedRmnContract field to OnRamp struct
  • Cache it once per OnRamp initialization
  • fix: Use correct cache function for StaticConfig
  • Add Benchmark before and after caching

Note: I did benchmark the caching function against using a map with the address as key and the function caching was a bit faster. Also using the caching is more consistent with the current implementation like StaticConfig.

Benchmark results:

BenchmarkIsSourceCursedWithCache-14        90266             12526 ns/op
BenchmarkIsSourceCursedWithCache-14        88867             12411 ns/op
BenchmarkIsSourceCursedWithCache-14        90930             12379 ns/op
BenchmarkIsSourceCursedWithCache-14        91108             12259 ns/op
BenchmarkIsSourceCursedWithCache-14        90915             12306 ns/op
BenchmarkIsSourceCursedOriginal-14          3002            397356 ns/op
BenchmarkIsSourceCursedOriginal-14          2996            402220 ns/op
BenchmarkIsSourceCursedOriginal-14          3009            399806 ns/op
BenchmarkIsSourceCursedOriginal-14          2575            397963 ns/op
BenchmarkIsSourceCursedOriginal-14          2596            396812 ns/op

* Add new cachedRmnContract field to OnRamp struct
* Cache it once per OnRamp initialization
* Add Benchmark before and after caching
Static config was being called with every call to `IsSourceCursed`

Needed to wrap the function call in `cache.CallOnceOnNoError` to activate the cache.
@github-actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@asoliman92 asoliman92 force-pushed the chore/CCIP-2391-cache-arm-contract branch from d7a2360 to eecc977 Compare May 31, 2024 19:15
@asoliman92 asoliman92 changed the title Cache RMN Contract in OnRamp 1_5 Cache RMN Contract in OnRamp May 31, 2024
@asoliman92 asoliman92 requested a review from 0xnogo June 3, 2024 06:29
@asoliman92 asoliman92 requested a review from dimkouv June 3, 2024 06:29
@asoliman92 asoliman92 marked this pull request as ready for review June 3, 2024 06:32
@asoliman92 asoliman92 requested a review from a team as a code owner June 3, 2024 06:32
[]common.Hash{configSetEventSig},
onRampAddress,
),
cachedStaticConfig: cachedStaticConfig,
Copy link
Contributor

@dimkouv dimkouv Jun 3, 2024

Choose a reason for hiding this comment

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

only difference is that it's not loaded on init?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@asoliman92 asoliman92 Jun 3, 2024

Choose a reason for hiding this comment

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

only difference is that it's not loaded on init?

Yes. When doing the benchmarks I found out that it was being called every time. Needed to wrap it within CallOnceOnNoError to actually cache it.

Comment on lines +98 to +99
cachedStaticConfig: cache.CallOnceOnNoError(cachedStaticConfig),
cachedRmnContract: cache.CallOnceOnNoError(cachedRmnContract),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these called upon creating a new CCIP job? The static config one makes an rpc call, if it errors, what happens? We want to avoid a job not starting up due to intermittent rpc errors, c.f #894

Copy link
Contributor Author

@asoliman92 asoliman92 Jun 3, 2024

Choose a reason for hiding this comment

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

Good question! I asked it myself when I started working on it initially as well. As you can see it's wrapped within CallOnceOnNoError, so in case of errors it will not actually cache. And as this is a function definition and not actually calling while starting, it is going to be called lazily when IsSourceCursed is called. That's why I think it shouldn't affect the job starting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, good note, so these aren't called here on these lines, but they will be called when IsSourceCursed is called, and only cached if there's no error. You're right, that doesn't block job startup.

@asoliman92 asoliman92 enabled auto-merge (squash) June 3, 2024 14:32
@asoliman92 asoliman92 merged commit be98879 into ccip-develop Jun 3, 2024
@asoliman92 asoliman92 deleted the chore/CCIP-2391-cache-arm-contract branch June 3, 2024 14:42
mateusz-sekara pushed a commit that referenced this pull request Jun 14, 2024
## Motivation

Every call to arm_contract.NewARMContract deserialized ARM contract abi
under the hood. Considering that OnRamp.IsSourceCursed is called very
often by the background worker it creates a huge CPU overhead. Please
initialize ARMContract only once and cache it instead of doing it with
every method call

## Solution

* Add new cachedRmnContract field to OnRamp struct
* Cache it once per OnRamp initialization
*  fix: Use correct cache function for StaticConfig
* Add Benchmark before and after caching

Note: I did benchmark the caching function against using a map with the
address as key and the function caching was a bit faster. Also using the
caching is more consistent with the current implementation like
`StaticConfig`.

Benchmark results:
```
BenchmarkIsSourceCursedWithCache-14        90266             12526 ns/op
BenchmarkIsSourceCursedWithCache-14        88867             12411 ns/op
BenchmarkIsSourceCursedWithCache-14        90930             12379 ns/op
BenchmarkIsSourceCursedWithCache-14        91108             12259 ns/op
BenchmarkIsSourceCursedWithCache-14        90915             12306 ns/op
BenchmarkIsSourceCursedOriginal-14          3002            397356 ns/op
BenchmarkIsSourceCursedOriginal-14          2996            402220 ns/op
BenchmarkIsSourceCursedOriginal-14          3009            399806 ns/op
BenchmarkIsSourceCursedOriginal-14          2575            397963 ns/op
BenchmarkIsSourceCursedOriginal-14          2596            396812 ns/op
```
asoliman92 added a commit that referenced this pull request Jul 31, 2024
## Motivation

Every call to arm_contract.NewARMContract deserialized ARM contract abi
under the hood. Considering that OnRamp.IsSourceCursed is called very
often by the background worker it creates a huge CPU overhead. Please
initialize ARMContract only once and cache it instead of doing it with
every method call

## Solution

* Add new cachedRmnContract field to OnRamp struct
* Cache it once per OnRamp initialization
*  fix: Use correct cache function for StaticConfig
* Add Benchmark before and after caching

Note: I did benchmark the caching function against using a map with the
address as key and the function caching was a bit faster. Also using the
caching is more consistent with the current implementation like
`StaticConfig`.

Benchmark results:
```
BenchmarkIsSourceCursedWithCache-14        90266             12526 ns/op
BenchmarkIsSourceCursedWithCache-14        88867             12411 ns/op
BenchmarkIsSourceCursedWithCache-14        90930             12379 ns/op
BenchmarkIsSourceCursedWithCache-14        91108             12259 ns/op
BenchmarkIsSourceCursedWithCache-14        90915             12306 ns/op
BenchmarkIsSourceCursedOriginal-14          3002            397356 ns/op
BenchmarkIsSourceCursedOriginal-14          2996            402220 ns/op
BenchmarkIsSourceCursedOriginal-14          3009            399806 ns/op
BenchmarkIsSourceCursedOriginal-14          2575            397963 ns/op
BenchmarkIsSourceCursedOriginal-14          2596            396812 ns/op
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants