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

dev: optimized bitshifts by using a lookup table for powers of two #984

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

augustin-v
Copy link

@augustin-v augustin-v commented Sep 26, 2024

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • [✅ ] Other (please describe): dev

What is the current behavior?

The bit shifting operations in the existing implementation are not optimized, leading to potential inefficiencies.

Resolves: #905

What is the new behavior?

  • Introduced a new implementation for bit shifting using a lookup table for powers of two.
  • Enhanced efficiency by minimizing multiplications and leveraging early returns for large shift values.

Does this introduce a breaking change?

  • Yes
  • [✅ ] No

This change is Reviewable

enitrat
enitrat previously approved these changes Sep 27, 2024
Copy link
Contributor

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Great!

For curiosity, if you're interested, you can do some profiling to see how many steps are saved when doing bitshifts. The biggest difference should be when shifting 255 bits as it saves a lot of steps by skipping the expensive exponenciation

https://foundry-rs.github.io/starknet-foundry/snforge-advanced-features/profiling.html

@enitrat enitrat dismissed their stale review September 27, 2024 09:53

edit: actually, tests don't pass because POW_2 only gives powers of two up to 128. You could introduce a new POW_2_256 variable with powers of two defined up to 2**255

@enitrat
Copy link
Contributor

enitrat commented Sep 27, 2024

The easiest way would be to use python to generate cairo code, like so:

def generate_powers_of_two():
    print("pub const POW_2_256: [")
    print("    u256")
    print("    ; 256] = [")

    for i in range(256):
        value = 2 ** i
        hex_value = f"0x{value:x}"
        print(f"    {hex_value},")

    print("];")

if __name__ == "__main__":
    generate_powers_of_two()

@augustin-v
Copy link
Author

Thank you for the review, I'll add a new constant as instructed and try profiling as well!

enitrat
enitrat previously approved these changes Sep 29, 2024
@augustin-v augustin-v closed this Sep 29, 2024
@enitrat enitrat reopened this Sep 29, 2024
manlikeHB and others added 6 commits September 30, 2024 09:59
* add validate_eth_tx test

* polish: fmt + docstrings
* First batch of replacements

* Second batch of replacements

* Clean commented code

* scarb fmt

* Apply code review recommendations
* ceil to bytes32

* overflow and nits
* load word to bytes

* unwrap and nits
* implement eth_get_balance

* add missing files

* made requested changes

* fmt

---------

Co-authored-by: enitrat <[email protected]>
* dev: use native in ci

* remove outdated gas reports

* fix fmt

* fix runtime location

* avoid clearing the ssj checkout

* use correct working dir

* update stack size when running rust ef-tests

* update workflows

* simplify workflows by avoiding re-building runtimes

* rework workflow structure
Gerson2102 and others added 4 commits September 30, 2024 17:52
* Implementation of eth_get_transaction_count function

* Refactoring validate nonce in validation.cairo

* Changing the deprecated function

* Validating nonce without new function

* Adding method to interface and test it

* fix tests

* fmt

---------

Co-authored-by: enitrat <[email protected]>
* fix: mulmod

* fmt
* fix: overflow in message_call_gas

* fmt
omahs and others added 3 commits October 1, 2024 10:25
* fix typos

* fix typo

* fix typos

* fix typo

* fix typo
* fix: saturate jumpi index taken on stack

* scout: remove print
dev: optimized bitshifts by using a lookup table for powers of two

tmp
@enitrat enitrat force-pushed the dev/905-optimized-bitshifts branch 2 times, most recently from 31b3096 to 8d0aa04 Compare October 1, 2024 13:30
@enitrat
Copy link
Contributor

enitrat commented Oct 1, 2024

hi @augustin-v it seems that the changes brought in scope of these PR highlight a memory-management related issue in Cairo Native which crashes our CI runner, thus I will have to keep it open until it is fixed

@augustin-v
Copy link
Author

Hi @enitrat, understood. Thank you for the update, and please let me know if there's anything I can do to assist with resolving the issue

@enitrat enitrat force-pushed the dev/905-optimized-bitshifts branch from 8d0aa04 to 189671b Compare October 1, 2024 16:54
enitrat and others added 4 commits October 1, 2024 18:59
* fix: conversion of stack values in opcode execution

* fix test
* test_kakarot_core_get_starknet_address

* ci: downgrade cairo native (kkrt-labs#1008)

* dev: use checked math (kkrt-labs#1009)

* fix get_starknet_address test

---------

Co-authored-by: Mathieu <[email protected]>
@enitrat
Copy link
Contributor

enitrat commented Oct 2, 2024

hey @augustin-v i ran some profiling on test_execute_from_outside_eip1559 - which is just an empty execution, so it only performs validation steps:

Before:
image

After:image

It's nearly 10x better :)

@augustin-v
Copy link
Author

augustin-v commented Oct 2, 2024

@enitrat wow it improved much more than I thought it would, bitshift operations looking good

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.

dev: optimize bitshifts by using a lookup table for powers of two
8 participants