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

[EraVM] Replace near_call with jump #653

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TerryGuo
Copy link
Collaborator

@TerryGuo TerryGuo commented Jul 8, 2024

Code Review Checklist

Purpose

Ticket Number

Requirements

  • Have the requirements been met?
  • Have stakeholder(s) approved the change?

Implementation

  • Does this code change accomplish what it is supposed to do?
  • Can this solution be simplified?
  • Does this change add unwanted compile-time or run-time dependencies?
  • Could an additional framework, API, library, or service improve the solution?
  • Could we reuse part of LLVM instead of implementing the patch or a part of it?
  • Is the code at the right abstraction level?
  • Is the code modular enough?
  • Can a better solution be found in terms of maintainability, readability, performance, or security?
  • Does similar functionality already exist in the codebase? If yes, why isn’t it reused?
  • Are there any best practices, design patterns or language-specific patterns that could substantially improve this code?

Logic Errors and Bugs

  • Can you think of any use case in which the
    code does not behave as intended?
  • Can you think of any inputs or external events
    that could break the code?

Error Handling and Logging

  • Is error handling done the correct way?
  • Should any logging or debugging information
    be added or removed?
  • Are error messages user-friendly?
  • Are there enough log events and are they
    written in a way that allows for easy
    debugging?

Maintainability

  • Is the code easy to read?
  • Is the code not repeated (DRY Principle)?
  • Is the code method/class not too long?

Dependencies

  • Were updates to documentation, configuration, or readme files made as required by this change?
  • Are there any potential impacts on other parts of the system or backward compatibility?

Security

  • Does the code introduce any security vulnerabilities?

Performance

  • Do you think this code change decreases
    system performance?
  • Do you see any potential to improve the
    performance of the code significantly?

Testing and Testability

  • Is the code testable?
  • Have automated tests been added, or have related ones been updated to cover the change?
    • For changes to mutable state
  • Do tests reasonably cover the code change (unit/integration/system tests)?
    • Line Coverage
    • Region Coverage
    • Branch Coverage
  • Are there some test cases, input or edge cases
    that should be tested in addition?

Readability

  • Is the code easy to understand?
  • Which parts were confusing to you and why?
  • Can the readability of the code be improved by
    smaller methods?
  • Can the readability of the code be improved by
    different function, method or variable names?
  • Is the code located in the right
    file/folder/package?
  • Do you think certain methods should be
    restructured to have a more intuitive control
    flow?
  • Is the data flow understandable?
  • Are there redundant or outdated comments?
  • Could some comments convey the message
    better?
  • Would more comments make the code more
    understandable?
  • Could some comments be removed by making the code itself more readable?
  • Is there any commented-out code?
  • Have you run a spelling and grammar checker?

Documentation

  • Is there sufficient documentation?
  • Is the ReadMe.md file up to date?

Best Practices

  • Follow Single Responsibility principle?
  • Are different errors handled correctly?
  • Are errors and warnings logged?
  • Magic values avoided?
  • No unnecessary comments?
  • Minimal nesting used?

Experts' Opinion

  • Do you think a specific expert, like a security
    expert or a usability expert, should look over
    the code before it can be accepted?
  • Will this code change impact different teams, and should they review the change as well?

Copy link

github-actions bot commented Jul 8, 2024

Benchmark results:

╔═╡ Size (-%) ╞════════════════╡ All M3B3 ╞═╗
║ Mean                               -0.168 ║
║ Best                                0.343 ║
║ Worst                             -19.005 ║
║ Total                              -0.443 ║
╠═╡ Cycles (-%) ╞══════════════╡ All M3B3 ╞═╣
║ Mean                                 -inf ║
║ Best                               11.364 ║
║ Worst                             -10.390 ║
║ Total                              -0.278 ║
╠═╡ Ergs (-%) ╞════════════════╡ All M3B3 ╞═╣
║ Mean                                0.112 ║
║ Best                                9.747 ║
║ Worst                              -1.224 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════════════╡ All MzB3 ╞═╗
║ Mean                               -0.394 ║
║ Best                                3.758 ║
║ Worst                             -37.086 ║
║ Total                              -1.052 ║
╠═╡ Cycles (-%) ╞══════════════╡ All MzB3 ╞═╣
║ Mean                                 -inf ║
║ Best                               11.111 ║
║ Worst                              -8.696 ║
║ Total                              -0.272 ║
╠═╡ Ergs (-%) ╞════════════════╡ All MzB3 ╞═╣
║ Mean                                0.163 ║
║ Best                               10.227 ║
║ Worst                              -2.257 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞═════╡ EVMInterpreter M3B3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╠═╡ Cycles (-%) ╞═══╡ EVMInterpreter M3B3 ╞═╣
║ Mean                               -0.880 ║
║ Best                                0.000 ║
║ Worst                              -2.005 ║
║ Total                              -0.422 ║
╠═╡ Ergs (-%) ╞═════╡ EVMInterpreter M3B3 ╞═╣
║ Mean                                0.049 ║
║ Best                                0.612 ║
║ Worst                              -0.038 ║
║ Total                               0.052 ║
╠═╡ Ergs/gas ╞══════╡ EVMInterpreter M3B3 ╞═╣
║ ADD                                46.750 ║
║ MUL                                28.050 ║
║ SUB                                46.750 ║
║ DIV                                30.450 ║
║ SDIV                               47.250 ║
║ MOD                                30.450 ║
║ SMOD                               43.650 ║
║ ADDMOD                             24.406 ║
║ MULMOD                             26.656 ║
║ EXP                                 7.537 ║
║ SIGNEXTEND                         29.250 ║
║ LT                                 50.750 ║
║ GT                                 50.750 ║
║ SLT                                74.750 ║
║ SGT                                72.750 ║
║ EQ                                 50.750 ║
║ ISZERO                             50.417 ║
║ AND                                46.750 ║
║ OR                                 46.750 ║
║ XOR                                46.750 ║
║ NOT                                46.417 ║
║ BYTE                               54.750 ║
║ SHL                                52.750 ║
║ SHR                                54.750 ║
║ SAR                                68.750 ║
║ SGT                                72.750 ║
║ SHA3                               28.243 ║
║ ADDRESS                            59.812 ║
║ BALANCE                            73.207 ║
║ ORIGIN                           1369.875 ║
║ CALLER                             59.812 ║
║ CALLVALUE                          59.812 ║
║ CALLDATALOAD                       48.750 ║
║ CALLDATASIZE                       60.125 ║
║ CALLDATACOPY                       75.092 ║
║ CODESIZE                           60.625 ║
║ CODECOPY                          135.349 ║
║ GASPRICE                         1372.688 ║
║ EXTCODESIZE                         5.103 ║
║ EXTCODECOPY                         5.153 ║
║ RETURNDATASIZE                     58.500 ║
║ RETURNDATACOPY                     46.556 ║
║ EXTCODEHASH                         8.228 ║
║ BLOCKHASH                         241.319 ║
║ COINBASE                         1372.875 ║
║ TIMESTAMP                        1369.875 ║
║ NUMBER                           1369.875 ║
║ PREVRANDAO                       1369.875 ║
║ GASLIMIT                         1375.875 ║
║ CHAINID                          1369.875 ║
║ SELFBALANCE                       645.625 ║
║ BASEFEE                          1369.875 ║
║ POP                                50.625 ║
║ MLOAD                              71.705 ║
║ MSTORE                             66.219 ║
║ MSTORE8                            76.010 ║
║ SLOAD                              26.676 ║
║ SSTORE                              9.012 ║
║ JUMP                               27.778 ║
║ JUMPI                              24.455 ║
║ PC                                 60.312 ║
║ MSIZE                              66.812 ║
║ GAS                                57.312 ║
║ JUMPDEST                          101.625 ║
║ PUSH0                              57.312 ║
║ PUSH1                              49.958 ║
║ PUSH2                              53.375 ║
║ PUSH4                              56.208 ║
║ PUSH5                              57.625 ║
║ PUSH6                              59.042 ║
║ PUSH7                              60.458 ║
║ PUSH8                              61.875 ║
║ PUSH9                              63.292 ║
║ PUSH10                             64.708 ║
║ PUSH11                             66.125 ║
║ PUSH12                             67.542 ║
║ PUSH13                             68.958 ║
║ PUSH14                             70.375 ║
║ PUSH15                             71.792 ║
║ PUSH16                             73.208 ║
║ PUSH17                             74.625 ║
║ PUSH18                             76.042 ║
║ PUSH19                             77.458 ║
║ PUSH20                             78.875 ║
║ PUSH21                             80.292 ║
║ PUSH22                             81.708 ║
║ PUSH23                             83.125 ║
║ PUSH24                             84.542 ║
║ PUSH25                             85.958 ║
║ PUSH26                             87.375 ║
║ PUSH27                             88.792 ║
║ PUSH28                             90.208 ║
║ PUSH29                             91.625 ║
║ PUSH30                             93.042 ║
║ PUSH31                             94.458 ║
║ PUSH32                             93.875 ║
║ DUP1                               40.417 ║
║ DUP2                               44.417 ║
║ DUP3                               42.417 ║
║ DUP4                               44.417 ║
║ DUP5                               44.417 ║
║ DUP6                               44.417 ║
║ DUP7                               44.417 ║
║ DUP8                               44.417 ║
║ DUP9                               44.417 ║
║ DUP10                              44.417 ║
║ DUP11                              44.417 ║
║ DUP12                              44.417 ║
║ DUP13                              44.417 ║
║ DUP14                              44.417 ║
║ DUP15                              44.417 ║
║ DUP16                              44.417 ║
║ SWAP1                              55.083 ║
║ SWAP2                              55.083 ║
║ SWAP3                              55.083 ║
║ SWAP4                              55.083 ║
║ SWAP5                              53.083 ║
║ SWAP6                              55.083 ║
║ SWAP7                              55.083 ║
║ SWAP8                              55.083 ║
║ SWAP9                              55.083 ║
║ SWAP10                             55.083 ║
║ SWAP11                             55.083 ║
║ SWAP12                             55.083 ║
║ SWAP13                             55.083 ║
║ SWAP14                             55.083 ║
║ SWAP15                             55.083 ║
║ SWAP16                             55.083 ║
║ CALL                               61.831 ║
║ STATICCALL                         61.821 ║
║ DELEGATECALL                       60.859 ║
║ CREATE                              5.416 ║
║ CREATE2                             7.631 ║
║ RETURN                              1.000 ║
║ REVERT                              1.000 ║
╠═╡ Ergs/gas (-%) ╞═╡ EVMInterpreter M3B3 ╞═╣
║ SHA3                                1.388 ║
║ BALANCE                             0.810 ║
║ ORIGIN                              0.400 ║
║ GASPRICE                            0.399 ║
║ EXTCODESIZE                         0.629 ║
║ EXTCODECOPY                         0.622 ║
║ EXTCODEHASH                        -0.112 ║
║ BLOCKHASH                           0.248 ║
║ COINBASE                            0.399 ║
║ TIMESTAMP                           0.400 ║
║ NUMBER                              0.400 ║
║ PREVRANDAO                          0.400 ║
║ GASLIMIT                            0.398 ║
║ CHAINID                             0.400 ║
║ SELFBALANCE                         0.730 ║
║ BASEFEE                             0.400 ║
║ SLOAD                               1.010 ║
║ SSTORE                              0.682 ║
║ CALL                               -0.074 ║
║ STATICCALL                         -0.074 ║
║ DELEGATECALL                       -0.082 ║
║ CREATE                             -0.016 ║
║ CREATE2                            -0.021 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞═════╡ EVMInterpreter MzB3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╠═╡ Cycles (-%) ╞═══╡ EVMInterpreter MzB3 ╞═╣
║ Mean                               -0.880 ║
║ Best                                0.000 ║
║ Worst                              -2.005 ║
║ Total                              -0.422 ║
╠═╡ Ergs (-%) ╞═════╡ EVMInterpreter MzB3 ╞═╣
║ Mean                                0.049 ║
║ Best                                0.612 ║
║ Worst                              -0.038 ║
║ Total                               0.052 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════╡ Precompiles M3B3 ╞═╗
║ Mean                               -0.202 ║
║ Best                                0.000 ║
║ Worst                              -0.985 ║
║ Total                              -0.151 ║
╠═╡ Cycles (-%) ╞══════╡ Precompiles M3B3 ╞═╣
║ Mean                               -0.628 ║
║ Best                                0.000 ║
║ Worst                              -1.253 ║
║ Total                              -0.009 ║
╠═╡ Ergs (-%) ╞════════╡ Precompiles M3B3 ╞═╣
║ Mean                                0.086 ║
║ Best                                0.213 ║
║ Worst                              -0.848 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════╡ Precompiles MzB3 ╞═╗
║ Mean                               -2.403 ║
║ Best                                0.000 ║
║ Worst                              -6.202 ║
║ Total                              -3.256 ║
╠═╡ Cycles (-%) ╞══════╡ Precompiles MzB3 ╞═╣
║ Mean                               -0.970 ║
║ Best                                0.000 ║
║ Worst                              -2.970 ║
║ Total                              -0.283 ║
╠═╡ Ergs (-%) ╞════════╡ Precompiles MzB3 ╞═╣
║ Mean                                0.824 ║
║ Best                                6.414 ║
║ Worst                              -0.825 ║
║ Total                               0.006 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞══════════╡ Real life M3B3 ╞═╗
║ Mean                               -0.675 ║
║ Best                                0.343 ║
║ Worst                             -19.005 ║
║ Total                              -0.873 ║
╠═╡ Cycles (-%) ╞════════╡ Real life M3B3 ╞═╣
║ Mean                               -0.773 ║
║ Best                                0.000 ║
║ Worst                              -2.182 ║
║ Total                              -0.441 ║
╠═╡ Ergs (-%) ╞══════════╡ Real life M3B3 ╞═╣
║ Mean                                0.128 ║
║ Best                                0.687 ║
║ Worst                              -1.224 ║
║ Total                               0.125 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞══════════╡ Real life MzB3 ╞═╗
║ Mean                               -3.215 ║
║ Best                                0.000 ║
║ Worst                             -21.964 ║
║ Total                              -4.210 ║
╠═╡ Cycles (-%) ╞════════╡ Real life MzB3 ╞═╣
║ Mean                               -1.145 ║
║ Best                                1.896 ║
║ Worst                              -4.730 ║
║ Total                              -1.124 ║
╠═╡ Ergs (-%) ╞══════════╡ Real life MzB3 ╞═╣
║ Mean                                0.361 ║
║ Best                                4.554 ║
║ Worst                              -2.232 ║
║ Total                               0.716 ║
╚═══════════════════════════════════════════╝

@TerryGuo TerryGuo force-pushed the terry-replace-nearcall-jcall branch 10 times, most recently from f9e6e25 to 9175c67 Compare July 14, 2024 12:49
@akiramenai akiramenai force-pushed the terry-replace-nearcall-jcall branch from 9175c67 to 32484f6 Compare July 25, 2024 09:42
Copy link

github-actions bot commented Jul 29, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@akiramenai akiramenai force-pushed the terry-replace-nearcall-jcall branch from 6787045 to b9ebaea Compare July 30, 2024 14:22
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