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!: using WithHash<T> in SharedMutable + fixing slot allocation #11716

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Feb 4, 2025

With the introduction of WithHash<T> it made sense to use it in SharedMutable as well. Given that I was dealing with storage I decided to tackle other SharedMutable related issues.

Fixes #5492
Fixes #5491

Copy link
Contributor Author

benesjan commented Feb 4, 2025

@benesjan benesjan changed the title feat: using WithHash<T> in SharedMutable + fixing slotallocation feat: using WithHash<T> in SharedMutable + fixing slot allocation Feb 4, 2025
@benesjan benesjan added the e2e-all CI: Enables this CI job. label Feb 4, 2025
@benesjan benesjan marked this pull request as ready for review February 4, 2025 12:42
@benesjan benesjan marked this pull request as draft February 4, 2025 15:20
@benesjan benesjan force-pushed the 08-15-_do_not_merge_feat_optimizing_sharedimmutable branch from bb6e548 to 30bfe9c Compare February 4, 2025 15:29
@benesjan benesjan force-pushed the 02-04-feat_using_withhash_t_in_sharedmutable_fixing_slotallocation branch 9 times, most recently from 5e7fc3f to 5553ac3 Compare February 5, 2025 09:47
@benesjan benesjan changed the title feat: using WithHash<T> in SharedMutable + fixing slot allocation feat!: using WithHash<T> in SharedMutable + fixing slot allocation Feb 5, 2025
@benesjan benesjan force-pushed the 08-15-_do_not_merge_feat_optimizing_sharedimmutable branch from faac2b8 to e6b47b8 Compare February 5, 2025 15:33
@benesjan benesjan force-pushed the 02-04-feat_using_withhash_t_in_sharedmutable_fixing_slotallocation branch 2 times, most recently from 3cb4e85 to 6f11f09 Compare February 5, 2025 15:35
Base automatically changed from 08-15-_do_not_merge_feat_optimizing_sharedimmutable to master February 5, 2025 16:10
@benesjan benesjan force-pushed the 02-04-feat_using_withhash_t_in_sharedmutable_fixing_slotallocation branch from 6f11f09 to 98c0571 Compare February 5, 2025 16:22
@benesjan benesjan marked this pull request as ready for review February 5, 2025 16:23
@AztecBot
Copy link
Collaborator

AztecBot commented Feb 5, 2025

Docs Preview

Hey there! 👋 You can check your preview at https://67a4aed4fdbdfc1bf677d60a--aztec-docs-dev.netlify.app

@benesjan benesjan requested a review from nventuro February 5, 2025 16:51
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Changes to public function bytecode sizes

Generated at commit: 57105b27bf97c5f9ac5198b564db4bf5f2150bba, compared to commit: 7820cb7b373370f16fed249beed07809da6998cc

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
TokenBlacklist::transfer_public -135 ✅ -2.43%
TokenBlacklist::shield -157 ✅ -2.80%
TokenBlacklist::burn_public -148 ✅ -2.99%
TokenBlacklist::public_dispatch -1,861 ✅ -7.27%
TokenBlacklist::mint_private -318 ✅ -8.57%
TokenBlacklist::mint_public -338 ✅ -9.57%
TokenBlacklist::get_roles -333 ✅ -12.31%
Auth::public_dispatch -1,787 ✅ -18.85%
TokenBlacklist::constructor -1,685 ✅ -25.05%
TokenBlacklist::update_roles -1,748 ✅ -27.24%
Auth::set_authorized_delay -1,889 ✅ -38.10%
Auth::set_authorized -1,887 ✅ -38.83%
Auth::get_authorized -883 ✅ -47.68%
Auth::get_scheduled_authorized -889 ✅ -50.68%
Auth::get_authorized_delay -1,299 ✅ -65.61%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
TokenBlacklist::transfer_public 5,430 (-135) -2.43%
TokenBlacklist::shield 5,445 (-157) -2.80%
TokenBlacklist::burn_public 4,807 (-148) -2.99%
TokenBlacklist::public_dispatch 23,725 (-1,861) -7.27%
TokenBlacklist::mint_private 3,394 (-318) -8.57%
TokenBlacklist::mint_public 3,194 (-338) -9.57%
TokenBlacklist::get_roles 2,372 (-333) -12.31%
Auth::public_dispatch 7,693 (-1,787) -18.85%
TokenBlacklist::constructor 5,041 (-1,685) -25.05%
TokenBlacklist::update_roles 4,669 (-1,748) -27.24%
Auth::set_authorized_delay 3,069 (-1,889) -38.10%
Auth::set_authorized 2,973 (-1,887) -38.83%
Auth::get_authorized 969 (-883) -47.68%
Auth::get_scheduled_authorized 865 (-889) -50.68%
Auth::get_authorized_delay 681 (-1,299) -65.61%

@@ -142,7 +144,7 @@ impl<let INITIAL_DELAY: u32> Packable<1> for ScheduledDelayChange<INITIAL_DELAY>
[packed.to_integer()]
}

fn unpack(input: [Field; 1]) -> Self {
fn unpack(input: [Field; SCHEDULED_DELAY_CHANGE_PCKD_LEN]) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Insted of defining packable for svc and sdc, we can create the struct that contains both and just implement packable for that. If we do this, we can combine sdc and svc, since svc has a block of change (32 bits) that can be merged into sdc. Then we'd have 2N+1 total instead of 2N+2, which makes public writes cheaper and private reads a bit cheaper (less hashing).

We can do this in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed 2 days ago that we want to be able to read svc and sdc individually in public. For this reason they each need to implement packable (storage_read requires it). Here is where we read it individually.

But when writing both values get written at once as they are wrapped in SharedMutableValues.

@benesjan benesjan marked this pull request as draft February 6, 2025 11:39
@benesjan benesjan force-pushed the 02-04-feat_using_withhash_t_in_sharedmutable_fixing_slotallocation branch from 8d1b517 to c386ccf Compare February 6, 2025 11:42
@benesjan benesjan requested a review from nventuro February 6, 2025 12:00
@benesjan benesjan removed the e2e-all CI: Enables this CI job. label Feb 6, 2025
@benesjan benesjan marked this pull request as ready for review February 6, 2025 12:01
@benesjan benesjan force-pushed the 02-04-feat_using_withhash_t_in_sharedmutable_fixing_slotallocation branch from 806e647 to 69aa267 Compare February 6, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants