Skip to content

Comments

Use uint64 for Page valid bitlist + Reuse hasher and buffers#14606

Merged
Inphi merged 25 commits intoethereum-optimism:developfrom
ec2:ec2/bitlist
Apr 30, 2025
Merged

Use uint64 for Page valid bitlist + Reuse hasher and buffers#14606
Inphi merged 25 commits intoethereum-optimism:developfrom
ec2:ec2/bitlist

Conversation

@ec2
Copy link
Contributor

@ec2 ec2 commented Mar 3, 2025

Description

There is currently a lot of heap allocation during Merklization and just keeping track of intermediate nodes. By reclaiming and reusing buffers when marking the tree's merkle cache as well as reusing the sha256 hasher, we can save on a lot of allocations.

In addition, in CachedPage, a bitlist is managed to keep track of whether intermediate nodes are valid or not. By using integers instead of an array of bools, we can do some bit magic to branchlessly maintain this bitlist. Also by doing this, we also avoid the Go runtime's bounds checking every time we mark a bit valid or not.

Tests

Additional context

Metadata

@ec2 ec2 requested review from a team as code owners March 3, 2025 21:47
@ec2 ec2 requested review from ControlCplusControlV and mds1 March 3, 2025 21:47
@ajsutton
Copy link
Contributor

ajsutton commented Mar 3, 2025

/ci authorize 5b202da

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

I think this looks good generally. Would be good to add some tests around the pooling and I'll see if I can get a second opinion from @mbaxter on the change from bools to bit mask as I'm notoriously bad at that kind of bit shift logic.

Copy link
Contributor

@mbaxter mbaxter left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, though I did have to stare at the bit math 😅

Left a few suggestions. One concern is that it looks like we're silently assuming the
PageSize value is fixed at 4096.

@opgitgovernance opgitgovernance added the S-stale Status: Will be closed unless there is activity label Mar 19, 2025
@opgitgovernance
Copy link
Contributor

This pr has been automatically marked as stale and will be closed in 5 days if no updates

@ec2 ec2 requested review from ajsutton and mbaxter March 28, 2025 21:30
@ajsutton
Copy link
Contributor

/ci authorize 633c322

@tynes tynes added S-exempt-stale Status: Do not mark stale and removed S-stale Status: Will be closed unless there is activity labels Apr 7, 2025
@pauldowman pauldowman requested a review from Inphi April 9, 2025 20:48
Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

Sorry for the precambrian-late review. Changes look good to me. Thanks!

@Inphi Inphi added this pull request to the merge queue Apr 30, 2025
Merged via the queue into ethereum-optimism:develop with commit ae6bb61 Apr 30, 2025
51 checks passed
iquidus pushed a commit to Layr-Labs/optimism that referenced this pull request Jul 24, 2025
…m-optimism#14606)

* Abstract the Merkle representation

* Implement asterisc's MPT

* migrate tests for mpt

* migrate tests for mpt

* copied benchmarks from asterisc

* fix failed merge

* Avoid pagelookup twice during setword invalidation

* fix state json codec test

* fix for singlethread too

* fix op-challenger test

* Remove MPT implementation

* address comments

* fix benchmark

* Use uint64 and also reuse hasher and buffers

* rename and fix lint

* getLowHighMask method as suggested

* compile time assertion of pagesize

* make HashPair use HashPairNodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-exempt-stale Status: Do not mark stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants