8000 Remove some needless allocations by elichai · Pull Request #422 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove some needless allocations #422

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
Merged
merged 3 commits into from
May 19, 2020
Merged

Remove some needless allocations #422

merged 3 commits into from
May 19, 2020

Conversation

elichai
Copy link
Member
@elichai elichai commented Apr 8, 2020

Hi,
I removed allocations in places that we serialized into a Vec just to then hash it. instead we can feed the hasher itself instead of a vector and serialize into that.

@elichai elichai mentioned this pull request Apr 8, 2020
Merged
dpc
dpc previously approved these changes Apr 8, 2020
View reviewed changes
@elichai elichai force-pushed the 2020-04-remove-alloc branch from cd51d00 to 99a6186 Compare April 12, 2020 16:04
stevenroose
stevenroose reviewed Apr 19, 2020
View reviewed changes
Copy link
Collaborator
@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

Very much concept ACK. I remember changing many of the Hash::hash usages into consensus encoding into the engine as well. All these are such obvious wins :)

@elichai elichai force-pushed the 2020-04-remove-alloc branch from 99a6186 to 83e9077 Compare April 20, 2020 08:12
@codecov-io
Copy link
codecov-io commented Apr 20, 2020 •
edited
Loading

Codecov Report

Merging #422 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
+ Coverage   86.71%   86.77%   +0.05%     
==========================================
  Files          40       40              
  Lines        8689     8988     +299     
==========================================
+ Hits         7535     7799     +264     
- Misses       1154     1189      +35
Impacted Files Coverage Δ
src/blockdata/transaction.rs 95.70% <100.00%> (+1.70%) ⬆️
src/consensus/encode.rs 89.50% <100.00%> (+0.26%) ⬆️
src/util/misc.rs 97.36% <100.00%> (+2.28%) ⬆️
src/util/psbt/map/output.rs 68.85% <0.00%> (+0.51%) ⬆️
src/internal_macros.rs 60.00% <0.00%> (+0.52%) ⬆️
src/util/psbt/map/global.rs 66.66% <0.00%> (+0.79%) ⬆️
src/util/psbt/map/input.rs 70.73% <0.00%> (+1.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a7d1b...83e9077. Read the comment docs.

stevenroose
stevenroose previously approved these changes Apr 20, 2020
View reviewed changes
Copy link
Collaborator
@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

utACK 83e9077

@stevenroose
Copy link
Collaborator

I restarted the nightly test that timed out. I'm pretty confident that it will be ok.

@elichai
Copy link
Member Author
elichai commented May 19, 2020

Travis passed but github doesn't recognize it for some reason, trying to close and re-open to trigger both github and travis

@elichai elichai closed this May 19, 2020
@elichai elichai reopened this May 19, 2020
sgeisler
sgeisler previously approved these changes May 19, 2020
View reviewed changes
@sgeisler
Copy link
Contributor

Maybe you can just git amend with an empty change set and force push to trigger travis? It would invalidate the reviews but that's not a big deal since we could just look at the diff and re-ACK if it's none.

@elichai elichai dismissed stale reviews from sgeisler and stevenroose via af31017 May 19, 2020 09:58
@elichai elichai force-pushed the 2020-04-remove-alloc branch from 83e9077 to af31017 Compare May 19, 2020 09:58
@elichai
Copy link
Member Author
elichai commented May 19, 2020 •
edited
Loading

arghh I force pushed an empty amend because github's status checks are really stubborn, closing-reopening didn't work.

@sgeisler @stevenroose Could you please re-approve?

stevenroose
stevenroose approved these changes May 19, 2020
View reviewed changes
Copy link
Collaborator
@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link
codecov-commenter commented May 19, 2020 •
edited
Loading

Codecov Report

Merging #422 into master will decrease coverage by 1.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
- Coverage   86.71%   85.49%   -1.23%     
==========================================
  Files          40       39       -1     
  Lines        8689     7540    -1149     
==========================================
- Hits         7535     6446    -1089     
+ Misses       1154     1094      -60
Impacted Files Coverage Δ
src/blockdata/transaction.rs 93.98% <100.00%> (-0.01%) ⬇️
src/consensus/encode.rs 89.21% <100.00%> (-0.03%) ⬇️
src/util/misc.rs 96.77% <100.00%> (+1.69%) ⬆️
src/util/psbt/map/output.rs 60.52% <0.00%> (-7.81%) ⬇️
src/util/psbt/map/global.rs 60.67% <0.00%> (-5.20%) ⬇️
src/util/psbt/map/input.rs 65.62% <0.00%> (-3.89%) ⬇️
src/util/bip32.rs 86.87% <0.00%> (-2.98%) ⬇️
src/blockdata/block.rs 81.69% <0.00%> (-2.02%) ⬇️
src/network/message.rs 94.77% <0.00%> (-1.95%) ⬇️
src/util/address.rs 85.52% <0.00%> (-0.93%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a7d1b...af31017. Read the comment docs.

@elichai elichai merged commit 1c88be4 into master May 19, 2020
@elichai elichai deleted the 2020-04-remove-alloc branch May 19, 2020 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenroose stevenroose stevenroose approved these changes

@sgeisler sgeisler sgeisler approved these changes

@dpc dpc dpc left review comments

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

Successfully merging this pull request may close these issues.
6 participants
@elichai @codecov-io @stevenroose @sgeisler @codecov-commenter @dpc
0