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 and Cleanup CScript::FindAndDelete #7907

Merged
merged 4 commits into from May 5, 2016

Conversation

Projects
None yet
6 participants
@pstratem
Contributor

pstratem commented Apr 19, 2016

This PR improves the worst-case behavior of CScript::FindAndDelete.

I have emphasized the obvious correctness of the algorithm in this commit.

Additionally I have done extensive fuzzing to ensure the result is identical to the current implementation.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Apr 20, 2016

Contributor

I highly prefer this to #7884...the original is kinda hard to read, but #7884 is impossible to reasonably convince yourself works, even if it probably does.
I benchmarked syncing on tmpfs with the final call to VerifySignature in CheckSig() disabled on this and #7884 and couldn't detect any difference in sync time through about 280k (just happened to be some blk* flies I had lying around went until there).

Contributor

TheBlueMatt commented Apr 20, 2016

I highly prefer this to #7884...the original is kinda hard to read, but #7884 is impossible to reasonably convince yourself works, even if it probably does.
I benchmarked syncing on tmpfs with the final call to VerifySignature in CheckSig() disabled on this and #7884 and couldn't detect any difference in sync time through about 280k (just happened to be some blk* flies I had lying around went until there).

@TheBlueMatt

View changes

Show outdated Hide outdated src/script/script.h
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 21, 2016

Contributor

utACK 6443169, but I'd prefer benchmarks before merge.

edit: To clarify: benchmarks that show an improvement

Contributor

dcousens commented Apr 21, 2016

utACK 6443169, but I'd prefer benchmarks before merge.

edit: To clarify: benchmarks that show an improvement

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 21, 2016

Member

@kazcw @dcousens It is only intended to improve the worst case behaviour, and @TheBlueMatt already benchmarked that it does not measurably affect the average case.

@pstratem If, after including Matt's suggestion, you drop the result.reserve() call, I think this will run with zero heap effect for non-match situations.

Member

sipa commented Apr 21, 2016

@kazcw @dcousens It is only intended to improve the worst case behaviour, and @TheBlueMatt already benchmarked that it does not measurably affect the average case.

@pstratem If, after including Matt's suggestion, you drop the result.reserve() call, I think this will run with zero heap effect for non-match situations.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Apr 21, 2016

Contributor

@sipa indeed you're right I've made that change

Contributor

pstratem commented Apr 21, 2016

@sipa indeed you're right I've made that change

Improve worst-case behavior of CScript::FindAndDelete
Thanks to Sergio Lerner for identifying this issue and suggesting this kind of solution.
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 22, 2016

Contributor

utACK d1d7775, looks good to merge

Contributor

dcousens commented Apr 22, 2016

utACK d1d7775, looks good to merge

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 28, 2016

Member

utACK d1d7775

Member

laanwj commented Apr 28, 2016

utACK d1d7775

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 5, 2016

Member

utACK d1d7775

Member

sipa commented May 5, 2016

utACK d1d7775

@laanwj laanwj merged commit d1d7775 into bitcoin:master May 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 5, 2016

Merge #7907: Optimize and Cleanup CScript::FindAndDelete
d1d7775 Improve worst-case behavior of CScript::FindAndDelete (Patrick Strateman)
e2a30bc Unit test for CScript::FindAndDelete (Gavin Andresen)
c0f660c Replace c-style cast with c++ style static_cast. (Patrick Strateman)
ec9ad5f Replace memcmp with std::equal in CScript::FindAndDelete (Patrick Strateman)

svost added a commit to svost/novacoin that referenced this pull request May 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment