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

lang: remove space inference via derive(Default) #1563

Closed
paul-schaaf opened this issue Mar 6, 2022 · 6 comments · Fixed by #1519
Closed

lang: remove space inference via derive(Default) #1563

paul-schaaf opened this issue Mar 6, 2022 · 6 comments · Fixed by #1519

Comments

@paul-schaaf
Copy link
Contributor

  • inefficient (it calls Default::default and then len)
  • useless for dynamic types
@Henry-E
Copy link
Contributor

Henry-E commented Mar 18, 2022

Neither of these things seem like sufficient reason to remove space inference from derive(Default)

  • it's inefficient but it's a quality of life improvement, assuming you're not using any dynamic types
  • Dynamic types needing manual space definitions is fine, it's just a byte per u8 or character in a string

Is this being replaced by any way to manually call this? Or some other way to simplify the calculation of how much space is needed. Like should we start adding LEN functions to all of our accounts structs now.

(this comment is probably coming too late but thought it worth responding anyway)

@paul-schaaf
Copy link
Contributor Author

paul-schaaf commented Mar 18, 2022

Dynamic types needing manual space definitions is fine, it's just a byte per u8 or character in a string

agree that this is not a big deal

but

it's inefficient

this is.

we want anchor to be as much of a zero-cost abstraction framework as possible and serializing an entire type at runtime just to get its static length is definitely far from that.

Like should we start adding LEN functions to all of our accounts structs now.

const variables (see the tests in this PR)

One thing Ive thought about is a #[len] macro that reads the types and writes the const variable. I'll create an issue for a discussion on this.

@paul-schaaf
Copy link
Contributor Author

#1647

@Henry-E
Copy link
Contributor

Henry-E commented Mar 19, 2022

Just how inefficient is it? There's quite a bit of mental overhead to calculating the size of different accounts and then keeping them up to date. It's sort of the equivalent of how comments and their underlying code sometimes stray from each other.

That said, I like that there's discussion of alternative solutions and I will comment there too. Thanks for listening!

@ceciEstErmat
Copy link
Contributor

Also, this seems to me to be a tool. If you need the efficiency then use space, if you do not care and want the quality of life then use Default.
Nothing in the framework enforced its use.

@paul-schaaf
Copy link
Contributor Author

pre txwide compute unit changes I wouldve agreed but now the efficiency of your instruction affects the efficiency of the entire transaction. Therefore, all instructions should be as efficient as possible.

But if you still want to use default to calculate the size, you can do it manually still

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 a pull request may close this issue.

3 participants