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.

Sign up for GitHub

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

Jump to bottom

Refactor: optimize Hashmap lib with BALLs #139

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

Refactor: optimize Hashmap lib with BALLs #139

wants to merge 5 commits into from

Conversation

eugenioclrc
Copy link
Contributor

@eugenioclrc eugenioclrc commented Apr 30, 2024 •
edited
Loading 

Ran 1 test suite in 58.31ms (54.73ms CPU time): 6 tests passed, 0
failed, 0 skipped (6 total tests)
testGetKey(bytes32) (gas: 0 (0.000%)) 
testGetKeys(bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKey(bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKeys(bytes32,bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKeys2D(bytes32,bytes32,bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKeys3D(bytes32,bytes32,bytes32,bytes32,bytes32) (gas: -9 (-0.026%)) 
Overall gas change: -9 (-0.006%)

@eugenioclrc
add evm_version to avoid NOT_ACTIVATED issues
9e9ac85
@eugenioclrc
Refactor to Hashmap using BALLs
3ccca49 
Gas snapshot;
```
forge snapshot --mc Hash --ffi  --fuzz-seed=11 --diff
[⠊] Compiling...
No files changed, compilation skipped

Ran 6 tests for test/data-structures/Hashmap.t.sol:HashmapTest
[PASS] testGetKey(bytes32) (runs: 1006, μ: 7550, ~: 7550)
[PASS] testGetKeys(bytes32,bytes32) (runs: 1006, μ: 7623, ~: 7623)
[PASS] testSetKey(bytes32,bytes32) (runs: 1006, μ: 28964, ~: 28984)
[PASS] testSetKeys(bytes32,bytes32,bytes32) (runs: 1006, μ: 29153, ~:
29153)
[PASS] testSetKeys2D(bytes32,bytes32,bytes32,bytes32) (runs: 1006, μ:
29497, ~: 29497)
[PASS] testSetKeys3D(bytes32,bytes32,bytes32,bytes32,bytes32) (runs:
1006, μ: 29888, ~: 29888)
Suite result: ok. 6 passed; 0 failed; 0 skipped; finished in 54.73ms
(226.84ms CPU time)

Ran 1 test suite in 58.31ms (54.73ms CPU time): 6 tests passed, 0
failed, 0 skipped (6 total tests)
testGetKey(bytes32) (gas: 0 (0.000%))
testGetKeys(bytes32,bytes32) (gas: 0 (0.000%))
testSetKey(bytes32,bytes32) (gas: 0 (0.000%))
testSetKeys(bytes32,bytes32,bytes32) (gas: 0 (0.000%))
testSetKeys2D(bytes32,bytes32,bytes32,bytes32) (gas: -12 (-0.041%))
testSetKeys3D(bytes32,bytes32,bytes32,bytes32,bytes32) (gas: -24
(-0.080%))
Overall gas change: -36 (-0.027%)

``
@Maddiaa0
Copy link
Member

Maddiaa0 commented May 1, 2024 •
edited
Loading

hahahha incredible, ill take a closer look at this tomorrow, tagging @Philogy for the vibes

@Maddiaa0 Maddiaa0 requested review from Maddiaa0 and Philogy May 1, 2024 00:17
@Philogy
Copy link
Collaborator

Philogy commented May 1, 2024

Hey, thanks for tagging @Maddiaa0. Exciting to see my BALLS in use. 😁

Philogy
Philogy reviewed May 1, 2024
View reviewed changes
src/data-structures/Hashmap.balls Outdated
// Load the data into memory.
mstore(add(mem_ptr, 0x20), slot)
mstore(mem_ptr, key1)
sha3(mem_ptr, 0x40)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the result of this hash needs to be stored and set into memory. Also wonder if you could save gas by storing add(mem_ptr, 0x20) in a var and just referencing that again

src/data-structures/Hashmap.balls Outdated
Comment on lines 50 to 54
sha3(mem_ptr, 0x40)

// put key2 in memory, before slot1
mstore(add(mem_ptr, 0x20), key2)
sha3(mem_ptr, 0x40)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly here you forget to save and write the result of the hash to memory

Copy link
Contributor Author

@eugenioclrc eugenioclrc May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that i mess up, gonna change current test to also predict the expected slot :D

@eugenioclrc
Add extra test to ensure slot is the expected
af93705 
`
@eugenioclrc
Fix missing hash operations
5c7b296 
```
testGetKey(bytes32) (gas: 0 (0.000%))
testGetKeys(bytes32,bytes32) (gas: 0 (0.000%))
testSetKey(bytes32,bytes32) (gas: 0 (0.000%))
testSetKeys(bytes32,bytes32,bytes32) (gas: 0 (0.000%))
testSetKeys2D(bytes32,bytes32,bytes32,bytes32) (gas: 0 (0.000%))
testSetKeys3D(bytes32,bytes32,bytes32,bytes32,bytes32) (gas: -9
(-0.026%))
Overall gas change: -9 (-0.006%)
``
@eugenioclrc
Copy link
Contributor Author

eugenioclrc commented May 1, 2024 •
edited
Loading

I have fix the hash operations, add some extra tests, and add @Philogy recommendations. I think its ready for a second look.

Spoiler alert, not a huge gain, but balls code seems cleaner and easier to read than the raw Huff.

testGetKey(bytes32) (gas: 0 (0.000%)) 
testGetKeys(bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKey(bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKeys(bytes32,bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKeys2D(bytes32,bytes32,bytes32,bytes32) (gas: 0 (0.000%)) 
testSetKeys3D(bytes32,bytes32,bytes32,bytes32,bytes32) (gas: -9 (-0.026%)) 
Overall gas change: -9 (-0.006%)

@eugenioclrc eugenioclrc requested a review from Philogy May 1, 2024 04:32
@eugenioclrc
Update SafeMath.huff
8f6cd1a 
remove `0x01 LABEL jumpi` for `LABEL jump`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Maddiaa0 Maddiaa0 Awaiting requested review from Maddiaa0

@Philogy Philogy Awaiting requested review from Philogy

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eugenioclrc @Maddiaa0 @Philogy