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

Optimize Witness Serialization #1071

Merged

Conversation

DanGould
Copy link
Contributor

fix #942

self.to_vec() allocates, it should be possible to avoid it - just feed the items into serializer.

based on #1068

@apoelstra
Copy link
Member

cargo +nightly build seems to fail -- I think you need to feature-gate the new serde stuff.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

GitHub does not let me comment on the commit log in line so commenting here.

The commit log is not correctly formatted. The subject should be separated from the body by a newline. (How are you making commits? Your tool should be able to be configured to warn you of this.)

I'm pretty anal about commit logs, I think they are important, they last for ever so are worth doing well. Here is the canonical blog post on the topic that I always link to: https://cbea.ms/git-commit/

I would prefer to see something like (notice subject then body (WHY then WHAT):

Optimize Witness serialization

Currently we allocate a new vector when serializing a Witness, this is inefficient and unnecessary. We can use serialize_seq to feed the witness elements directly into the serializer.

Optimize Witness serialization by removing the allocation.

We allocated a new vector when serializing a `Witness`. That was
inefficient and unnecessary. Use `serialize_seq` to feed the witness
elements directly into the serializer.

Optimize `Witness` serialization by removing the allocation.
@DanGould DanGould force-pushed the optimize-witness-serialization branch from 0912593 to 9bf9591 Compare June 29, 2022 09:10
@DanGould
Copy link
Contributor Author

I see the one import was already gated and wrongly assumed that would apply to every line that came after. this might work at the top as well:

#[cfg(features = "serde")]
use serde::{self, ser::SerializeSeq};

but I matched the imports with @tcharding's Human readable pr so as not to conflict.

This was supposed to be trivial merge so I don't have to ask for ci runtime, but I learned about feature gates and why git rebase reword highlighted red on me. fixed.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 9bf9591

(under the condition CI succeeds)

Thanks!

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 9bf9591

@apoelstra apoelstra merged commit 4f5fcd3 into rust-bitcoin:master Jun 29, 2022
@apoelstra
Copy link
Member

The commit message is fixed BTW to use Tobin's suggestion.

@tcharding
Copy link
Member

Thanks @DanGould.

@DanGould DanGould deleted the optimize-witness-serialization branch June 30, 2022 04:29
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
9bf9591 Optimize Witness Serialization (DanGould)

Pull request description:

  fix #942
  > self.to_vec() allocates, it should be possible to avoid it - just feed the items into serializer.

  based on rust-bitcoin/rust-bitcoin#1068

ACKs for top commit:
  Kixunil:
    ACK 9bf9591
  apoelstra:
    ACK 9bf9591

Tree-SHA512: 14553dfed20aee50bb6361d44986b38556cbb3e112e1b4d9e3b401c3da831b6bdf159089966254cf371c225ae929fc78516c96a6114b40a7bc1fda7305295e4a
@Kixunil Kixunil added the perf Performance optimizations/issues label Aug 1, 2022
@Kixunil Kixunil added this to the 0.29.0 milestone Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf Performance optimizations/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize Witness serialization
4 participants