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

Avoid temporary Vecs when serialising objects with discriminators #2691

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Avoid temporary Vecs when serialising objects with discriminators #2691

merged 5 commits into from
Dec 14, 2023

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Oct 31, 2023 •
edited
Loading

When serialising objects prefixed by a discriminator, rather then
using BorshSerialize::try_to_vec to create a temporary vector that is
appended to the output vector, create the output in advance, add
discriminator to it and then use BorshSerialize::serialize to
serialise the object directly to it.

Issue: #2231

@vercel
Copy link

vercel bot commented Oct 31, 2023

@mina86 is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

acheroncrypto
acheroncrypto reviewed Oct 31, 2023
View reviewed changes
lang/attribute/event/src/lib.rs Outdated Show resolved Hide resolved
lang/attribute/event/src/lib.rs Outdated Show resolved Hide resolved
@mina86
Copy link
Contributor Author

mina86 commented Oct 31, 2023 •
edited
Loading

I’m now getting failure in test_current_version somehow. O_o

thread 'tests::test_current_version' panicked at 'called `Result::unwrap()` on an `Err` value: Could not parse version file: empty string, expected a semver version', avm/src/lib.rs:378:31

@acheroncrypto
Copy link
Collaborator

I’m now getting failure in test_current_version somehow. O_o

thread 'tests::test_current_version' panicked at 'called `Result::unwrap()` on an `Err` value: Could not parse version file: empty string, expected a semver version', avm/src/lib.rs:378:31

That's strange but I'm assuming that's only locally since avm tests in CI pass.

avm error is definitely not related to this PR but the following is:

Can retrieve events when simulating a transaction:

AssertionError: expected [ Array(7) ] to deeply equal [ Array(7) ]
+ expected - actual

[
   "Program 3TEqcc8xhrhdspwbvoamUJe2borm4Nr72JxL66k6rgrh invoke [1]"
   "Program log: Instruction: TestSimulate"
-  "Program data: NgyCA9omwbMsAAAA"
-  "Program data: fPhuIELK/k7SBAAA"
-  "Program data: jvbowsvlmkcJAAAA"
-  "Program data: zxM5neEnS1kBAgMEBQYHCAkK"
-  "Program data: g06Ei2GL1gIBAgMEBQYHCAkKCw=="
+  "Program data: NgAAAAwAAACCAAAAAwAAANoAAAAmAAAAwQAAALMAAAAsAAAA"
+  "Program data: fAAAAPgAAABuAAAAIAAAAEIAAADKAAAA/gAAAE4AAADSBAAA"
+  "Program data: jgAAAPYAAADoAAAAwgAAAMsAAADlAAAAmgAAAEcAAAAJAAAA"
+  "Program data: zwAAABMAAAA5AAAAnQAAAOEAAAAnAAAASwAAAFkAAAABAgMEBQYHCAkK"
+  "Program data: gwAAAE4AAACEAAAAiwAAAGEAAACLAAAA1gAAAAIAAAABAgMEBQYHCAkKCw=="
]

CI logs: https://github.com/coral-xyz/anchor/actions/runs/6705905390/job/18221452799?pr=2691#step:10:610

@mina86
Copy link
Contributor Author

mina86 commented Oct 31, 2023

All green.

@mina86 mina86 changed the title Avoid Vec allocation in Event::data derived implementation Avoid temporary Vecs when serialising objects with discriminators Nov 1, 2023
@mina86
Avoid temporary Vecs when serialising objects with discriminators
63e699b 
When serialising objects prefixed by a discriminator, rather then
using BorshSerialize::try_to_vec to create a temporary vector that is
appended to the output vector, create the output in advance, add
discriminator to it and then use BorshSerialize::serialize to
serialise the object directly to it.

Issue: coral-xyz#2231
acheroncrypto
acheroncrypto reviewed Dec 12, 2023
View reviewed changes
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

I agree that it makes sense to set the default allocation to 256 instead of 1024 for most programs.

Overall, the changes look good to me but I'd like to play with it locally before merging.

cli/src/lib.rs
Comment on lines -3847 to +3849
let mut data = anchor_lang::idl::IDL_IX_TAG.to_le_bytes().to_vec();
data.append(&mut ix_inner.try_to_vec()?);
let mut data = Vec::with_capacity(256);
data.extend_from_slice(&anchor_lang::idl::IDL_IX_TAG.to_le_bytes());
ix_inner.serialize(&mut data)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We serialize 1000 bytes per each IDL Write instruction(most IDLs are bigger):

anchor/cli/src/lib.rs

Line 2397 in 4c0af6d

const MAX_WRITE_SIZE: usize = 1000;

CLI and client changes are negligible but I'm onboard with including them for the sake of consistency.

@acheroncrypto
Copy link
Collaborator

Yep, this is much better. Could you note this change in the CHANGELOG?

@mina86
Copy link
Contributor Author

mina86 commented Dec 14, 2023

Done.

acheroncrypto
acheroncrypto approved these changes Dec 14, 2023
View reviewed changes
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thank you!

@acheroncrypto acheroncrypto merged commit 250fa8c into coral-xyz:master Dec 14, 2023
47 of 48 checks passed
@mina86 mina86 deleted the a branch December 14, 2023 22:02
This was referenced Dec 16, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acheroncrypto acheroncrypto acheroncrypto approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@mina86 @acheroncrypto