Implementing get_size for everything #197

blockiosaurus · 2024-11-25T19:43:06Z

Fully implementing get_size and switching to that over serialized data size.

…lized data size.

vercel · 2024-11-25T19:43:10Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mpl-core-js-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 10, 2024 3:50pm

nhanphan · 2024-11-25T21:21:32Z

Is there a way to add a test to check the sizes are actually what we expect? (Eg they match with the plugin header)

blockiosaurus · 2024-11-30T20:35:37Z

You're right. We should probably have serialization size tests for every plugin.

danenbm

Looked at the trait implementations. Still need to look at processor code.

danenbm · 2024-12-03T00:26:43Z

programs/mpl-core/src/plugins/internal/authority_managed/attributes.rs

+        4 + self
+            .attribute_list
+            .iter()
+            .fold(0, |acc, attr| acc + attr.get_size())


Nit: This is elegant but I think using map and sum could be more simple, i.e.:

self .attribute_list .map(|attr| attr.get_size()) .sum::<usize>()

Nice! That's much better and same in CUs.

danenbm · 2024-12-03T00:33:26Z

programs/mpl-core/src/plugins/internal/authority_managed/royalties.rs

+    ProgramAllowList(Vec<Pubkey>), // 4
    /// Deny list of programs that are not allowed to transfer, receive, or send the asset.
-    ProgramDenyList(Vec<Pubkey>),
+    ProgramDenyList(Vec<Pubkey>), // 4


Nit: consistency, sometimes the commented length is the initial/default length of zero, other times it is listed as like 4 + len * Object

danenbm · 2024-12-03T00:35:29Z

programs/mpl-core/src/plugins/internal/authority_managed/royalties.rs

+
+impl DataBlob for Royalties {
+    fn get_initial_size() -> usize {
+        1


Shouldn't this be 7, or 2 + 4 + RuleSet::get_initial_size()?

danenbm · 2024-12-03T00:40:59Z

programs/mpl-core/src/plugins/internal/authority_managed/verified_creators.rs

+    }
+
+    fn get_size(&self) -> usize {
+        4 + self.signatures.len() * VerifiedCreatorsSignature::get_initial_size()


Nit: It happens to be fine since its fixed size but I think technically better to use get_size() here rhather than get_initial_size().

danenbm · 2024-12-03T00:45:51Z

programs/mpl-core/src/plugins/lifecycle.rs

@@ -8,7 +8,7 @@ use crate::{
    state::{Authority, Key, UpdateAuthority},
 };

-use crate::plugins::{
+use super::{


Nit: I thought in another PR you switched all the super to crate?

danenbm · 2024-12-03T00:47:52Z

programs/mpl-core/src/plugins/mod.rs

@@ -88,6 +88,38 @@ impl Plugin {

 impl Compressible for Plugin {}

+impl DataBlob for Plugin {
+    fn get_initial_size() -> usize {
+        1


is this really 1 or since there's no None, is it each plugin's initial size?

1 + match self { Plugin::Royalties(royalties) => royalties.get_initial_size() ...

There's still one byte required for the plugin enum discriminator.

danenbm · 2024-12-03T00:57:11Z

programs/mpl-core/src/plugins/mod.rs

+impl From<PluginType> for usize {
+    fn from(plugin_type: PluginType) -> Self {
+        plugin_type as usize
+    }
+}


Don't we normally do this with #[derive(ToPrimitive)]

danenbm · 2024-12-03T00:59:27Z

programs/mpl-core/src/plugins/plugin_registry.rs

+        2 + 1 + 8
+    }
+
+    fn get_size(&self) -> usize {
+        2 + self.authority.get_size() + 8


PluginType should be 1? But actually isn't it better to use PluginType::get_initial_size() and get_size()?

Even for Authority could use self.authority.get_initial_size() as well?

Yup you're right.

danenbm · 2024-12-03T01:05:34Z

programs/mpl-core/src/plugins/utils.rs

-    let core = T::load(account, 0)?;
-    let header_offset = core.get_size();
    let plugin_type = plugin.into();
-    let plugin_data = plugin.try_to_vec()?;
-    let plugin_size = plugin_data.len();
+    let plugin_size = plugin.get_size();


danenbm

Looks good but I agree with both of you we should add that testing now.

…dation/mpl-core into feat/impl-get-size

This reverts commit 84758e9.

…foundation/mpl-core into feat/impl-get-size

github-actions

Benchmark

Benchmark suite	Current: `15b9054`	Previous: `729fde8`	Ratio
`CU: create a new, empty asset`	`11182` Compute Units	`11082` Compute Units	`1.01`
`Space: create a new, empty asset`	`91` Bytes	`91` Bytes	`1`
`CU: create a new, empty asset with empty collection`	`22655` Compute Units	`22555` Compute Units	`1.00`
`Space: create a new, empty asset with empty collection`	`91` Bytes	`91` Bytes	`1`
`CU: create a new asset with plugins`	`35057` Compute Units	`40123` Compute Units	`0.87`
`Space: create a new asset with plugins`	`194` Bytes	`194` Bytes	`1`
`CU: create a new asset with plugins and empty collection`	`41079` Compute Units	`46145` Compute Units	`0.89`
`Space: create a new asset with plugins and empty collection`	`194` Bytes	`194` Bytes	`1`
`CU: list an asset`	`28009` Compute Units	`30237` Compute Units	`0.93`
`CU: sell an asset`	`39437` Compute Units	`38824` Compute Units	`1.02`
`CU: list an asset with empty collection`	`35567` Compute Units	`37795` Compute Units	`0.94`
`CU: sell an asset with empty collection`	`51765` Compute Units	`51152` Compute Units	`1.01`
`CU: list an asset with collection royalties`	`36336` Compute Units	`38499` Compute Units	`0.94`
`CU: sell an asset with collection royalties`	`56690` Compute Units	`56077` Compute Units	`1.01`
`CU: transfer an empty asset`	`6420` Compute Units	`6320` Compute Units	`1.02`
`CU: transfer an empty asset with empty collection`	`9018` Compute Units	`8918` Compute Units	`1.01`
`CU: transfer an asset with plugins`	`14719` Compute Units	`14604` Compute Units	`1.01`
`CU: transfer an asset with plugins and empty collection`	`17317` Compute Units	`17202` Compute Units	`1.01`

This comment was automatically generated by workflow using github-action-benchmark.

…into feat/impl-get-size

Implementing get_size for everything and switching to that over seria…

2274735

…lized data size.

blockiosaurus requested review from nhanphan and danenbm November 25, 2024 19:43

Removing WIP line.

33288ba

vercel bot deployed to Preview November 25, 2024 19:45 View deployment

danenbm reviewed Dec 3, 2024

View reviewed changes

danenbm reviewed Dec 5, 2024

View reviewed changes

Adding job summary.

84758e9

vercel bot deployed to Preview December 6, 2024 17:07 View deployment

blockiosaurus added 2 commits December 6, 2024 12:21

Revert change.

5243ba0

Merge branch 'feat/impl-get-size' of https://github.com/metaplex-foun…

d25710d

…dation/mpl-core into feat/impl-get-size

vercel bot deployed to Preview December 6, 2024 17:23 View deployment

Revert "Adding job summary."

d8aae30

This reverts commit 84758e9.

vercel bot deployed to Preview December 6, 2024 17:24 View deployment

Minor refactor to change name and make base size a constant.

55f12f6

vercel bot deployed to Preview December 6, 2024 21:31 View deployment

Merge branch 'feat/benchmark-summary' of https://github.com/metaplex-…

f35402b

…foundation/mpl-core into feat/impl-get-size

vercel bot deployed to Preview December 6, 2024 21:33 View deployment

github-actions bot reviewed Dec 6, 2024

View reviewed changes

Adding program tests.

146c593

vercel bot deployed to Preview December 9, 2024 22:04 View deployment

Fixing missing file.

1e9f3aa

vercel bot deployed to Preview December 9, 2024 22:15 View deployment

Merge branch 'main' of https://github.com/metaplex-foundation/mpl-core …

8c3e9d9

…into feat/impl-get-size

vercel bot deployed to Preview December 10, 2024 15:21 View deployment

Fixing wrong call.

e97959b

vercel bot deployed to Preview December 10, 2024 15:27 View deployment

Removing unnecessary impl.

15b9054

vercel bot deployed to Preview December 10, 2024 15:50 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing get_size for everything #197

Implementing get_size for everything #197

blockiosaurus commented Nov 25, 2024

vercel bot commented Nov 25, 2024 •

edited

Loading

nhanphan commented Nov 25, 2024

blockiosaurus commented Nov 30, 2024

danenbm left a comment

danenbm Dec 3, 2024

blockiosaurus Dec 6, 2024

danenbm Dec 3, 2024

danenbm Dec 3, 2024

danenbm Dec 3, 2024

danenbm Dec 3, 2024

danenbm Dec 3, 2024

blockiosaurus Dec 6, 2024

danenbm Dec 3, 2024

danenbm Dec 3, 2024

blockiosaurus Dec 6, 2024

danenbm Dec 3, 2024

danenbm left a comment

github-actions bot left a comment •

edited

Loading

Implementing get_size for everything #197

Are you sure you want to change the base?

Implementing get_size for everything #197

Conversation

blockiosaurus commented Nov 25, 2024

vercel bot commented Nov 25, 2024 • edited Loading

nhanphan commented Nov 25, 2024

blockiosaurus commented Nov 30, 2024

danenbm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danenbm left a comment

Choose a reason for hiding this comment

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Benchmark

vercel bot commented Nov 25, 2024 •

edited

Loading

github-actions bot left a comment •

edited

Loading