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

feat/perf: abi benchmarks #57

Merged
merged 1 commit into from
Jun 7, 2023
Merged

feat/perf: abi benchmarks #57

merged 1 commit into from
Jun 7, 2023

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented May 31, 2023

Results

sol-types: good
dyn-abi: not so good, optimizations needed, probably in DynSolType::encode_single

ethabi/encode/single    time:   [86.689 ns 87.177 ns 87.735 ns]
ethabi/encode/struct    time:   [172.32 ns 172.65 ns 173.02 ns]
ethabi/decode/word      time:   [33.044 ns 33.168 ns 33.310 ns]
ethabi/decode/dynamic   time:   [72.122 ns 72.273 ns 72.452 ns]

dyn-abi/encode/single   time:   [91.784 ns 92.412 ns 93.107 ns]
dyn-abi/encode/struct   time:   [724.03 ns 729.16 ns 734.79 ns]
dyn-abi/decode/word     time:   [72.234 ns 72.568 ns 72.967 ns]
dyn-abi/decode/dynamic  time:   [99.585 ns 99.764 ns 99.945 ns]

sol-types/encode/single time:   [43.869 ns 44.193 ns 44.581 ns]
sol-types/encode/struct time:   [55.131 ns 55.286 ns 55.423 ns]
sol-types/decode/word   time:   [1.3890 ns 1.4000 ns 1.4141 ns]
sol-types/decode/dynamic
                        time:   [36.848 ns 36.983 ns 37.135 ns]

@DaniPopes DaniPopes requested a review from prestwich May 31, 2023 11:30
@prestwich
Copy link
Member

can you also bench these against the previous implementation?

@DaniPopes
Copy link
Member Author

DaniPopes commented May 31, 2023

can you also bench these against the previous implementation?

which previous implementation? If you mean the one before my changes in #52, there is no difference in ABI logic/performance

@prestwich
Copy link
Member

I mean the abi impl relied on by ethers@2.0.0

@DaniPopes
Copy link
Member Author

that is ethabi

@prestwich
Copy link
Member

oh i'm just blind. got it. thank you

@DaniPopes
Copy link
Member Author

DaniPopes commented Jun 1, 2023

After 281cec3:

dyn-abi/encode/single   time:   [69.210 ns 69.292 ns 69.382 ns]                                  
                        change: [-26.237% -25.649% -25.227%] (p = 0.00 < 0.05)
                        Performance has improved.
dyn-abi/encode/struct   time:   [626.60 ns 627.97 ns 629.56 ns]                                   
                        change: [-12.694% -12.201% -11.654%] (p = 0.00 < 0.05)
                        Performance has improved.

dyn-abi/decode/word     time:   [49.180 ns 49.331 ns 49.572 ns]                                 
                        change: [-33.199% -32.358% -31.439%] (p = 0.00 < 0.05)
                        Performance has improved.
dyn-abi/decode/dynamic  time:   [84.247 ns 84.431 ns 84.620 ns]                                   
                        change: [-18.215% -17.543% -17.087%] (p = 0.00 < 0.05)
                        Performance has improved.

@DaniPopes DaniPopes changed the title feat: abi benchmarks feat/perf: abi benchmarks Jun 1, 2023
@prestwich
Copy link
Member

well, let's keep investigating :)

@DaniPopes
Copy link
Member Author

Maybe we can skip tokenizing by encoding directly the DynSolValue? Looking at the "struct" bench, ethabi is much more concise because of ethabi::encode(tokens: &[ethabi::Token]), while dyn-abi has to declare all the types, and then also clone the input on each call.

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

approving this. can you rebase, and open issues for future optimization work?

@DaniPopes DaniPopes merged commit 64bf519 into main Jun 7, 2023
@DaniPopes DaniPopes deleted the dani/abi-benches branch June 7, 2023 16:36
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.

2 participants