Skip to content

Make CScript (and prevector) c++11 movable. #9349

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
sipa merged 1 commit into from
Dec 27, 2016
Merged

Make CScript (and prevector) c++11 movable. #9349

sipa merged 1 commit into from
Dec 27, 2016

Conversation

@sipa
Copy link
Member

@sipa sipa commented Dec 14, 2016

Such moves are used when reallocating vectors that contain them, and could be used for more things later.

This change avoids an extra allocation when the data is not inlined.

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Dec 14, 2016

utAck 7ad1126

minor nits:

  • is it necessary to kill the CScript copy constructor in this pull?
  • you have an #include "assert.h" in prevector.h that seems to be unused.

@sipa
Copy link
Member Author

sipa commented Dec 14, 2016

is it necessary to kill the CScript copy constructor in this pull?

Yes, the presence of a custom copy constructor in CScript prevents the creation of a default move constructor or move assignment operator. I can add those as well, but given that the default copy constructor is perfectly appropriate, it's better to just pick that one, and get all default constructors and assignment operators.

you have an #include "assert.h" in prevector.h that seems to be unused.

Good catch.

dcousens
dcousens reviewed Dec 14, 2016
View reviewed changes
src/test/prevector_tests.cpp Outdated
Copy link
Contributor

@dcousens dcousens Dec 14, 2016

Choose a reason for hiding this comment

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

trivial question, but what are these values representing?

Copy link
Member Author

@sipa sipa Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Just things that are tested with certain percentage chance. The numbers here are chosen so that the copy/move/swap never occur at once (but it hardly matters).

Copy link
Contributor

@dcousens dcousens Dec 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

[still trivial] @sipa any reason not to document those percentages? Or perhaps represent them in a non-bit shifting format?
I find this to be the case often throughout, and I'm not the biggest fan of having to spend review time on determining the bit shifts in my head compared to higher level issues.

Trivial, but, prolific enough I thought I'd ask.

jonasschnelli
jonasschnelli approved these changes Dec 15, 2016
View reviewed changes
Copy link
Contributor

@jonasschnelli jonasschnelli left a comment

Choose a reason for hiding this comment

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

Code Review ACK 7ad1126215c0a527182840b2a98e5f83ddfad880.

nits:

  • new assert.h include can be removed
  • missing test for CScript move

gmaxwell
gmaxwell reviewed Dec 16, 2016
View reviewed changes
src/prevector.h Outdated
Copy link
Contributor

@gmaxwell gmaxwell Dec 16, 2016

Choose a reason for hiding this comment

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

no return statement in function returning non-void.

Copy link
Member Author

@sipa sipa Dec 16, 2016

Choose a reason for hiding this comment

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

Fixed.

@sipa
Copy link
Member Author

sipa commented Dec 16, 2016

@jonasschnelli Fair enough, there is no explicit test for CScript move, but I think we can rely on the C++ default constructors being correctly implemented in the compiler...

@theuni
Copy link
Member

theuni commented Dec 16, 2016

utACK other than the nits, looks familiar :)

@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 17, 2016

tested ACK.

laanwj
laanwj reviewed Dec 19, 2016
View reviewed changes
src/prevector.h Outdated
Copy link
Member

@laanwj laanwj Dec 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Does assert.h need to be included here? This doesn't seem to add any assertions (ah, I see @jonasschnelli also mentions this).

Copy link
Member Author

@sipa sipa Dec 22, 2016

Choose a reason for hiding this comment

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

Good point, fixed.

@laanwj
Copy link
Member

laanwj commented Dec 19, 2016

utACK efc0c4c, but see the nit

@sipa
Make CScript (and prevector) c++11 movable.
2ddfcfd 
Such moves are used when reallocating vectors that contain them,
for example.
@sipa
Copy link
Member Author

sipa commented Dec 22, 2016

Nit fixed.

@sipa sipa merged commit 2ddfcfd into bitcoin:master Dec 27, 2016
sipa added a commit that referenced this pull request Dec 27, 2016
@sipa
Merge #9349: Make CScript (and prevector) c++11 movable.
2db4cbc 
2ddfcfd Make CScript (and prevector) c++11 movable. (Pieter Wuille)
codablock pushed a commit to codablock/dash that referenced this pull request Jan 18, 2018
@sipa @codablock
Merge bitcoin#9349: Make CScript (and prevector) c++11 movable.
cbbab00 
2ddfcfd Make CScript (and prevector) c++11 movable. (Pieter Wuille)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@sipa @andvgal
Merge bitcoin#9349: Make CScript (and prevector) c++11 movable.
0b3b319 
2ddfcfd Make CScript (and prevector) c++11 movable. (Pieter Wuille)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
@sipa @CryptoCentric
Merge bitcoin#9349: Make CScript (and prevector) c++11 movable.
09b4290 
2ddfcfd Make CScript (and prevector) c++11 movable. (Pieter Wuille)
@sickpig sickpig mentioned this pull request Oct 24, 2019
Merged
@laanwj laanwj mentioned this pull request Nov 19, 2019
Closed
@random-zebra random-zebra mentioned this pull request Apr 24, 2020
Merged
1 task
Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request May 19, 2020
@Fuzzbawls
Merge #1557: [Core] Prevector
8dfc480 
a8b965e Explicitly initialize prevector _union (random-zebra)
bdd98e8 [Trivial] Add a comment on the use of prevector in script. (random-zebra)
bbaa6e3 Clarify prevector::erase and avoid swap-to-clear (random-zebra)
28a8435 prevector: assert successful allocation (random-zebra)
de41cb5 Only call clear on prevector if it isn't trivially destructible (random-zebra)
8784df3 Make CScript (and prevector) c++11 movable. (random-zebra)
83f9ac6 serialize: Deprecate `begin_ptr` / `end_ptr` (random-zebra)
c3ecc12 prevector: add C++11-like data() method (random-zebra)
5490aa0 test prevector::swap (random-zebra)
035760e prevector::swap: fix (unreached) data corruption (random-zebra)
0e71400 prevector: destroy elements only via erase() (random-zebra)
9811a68 [Core] Prevector type (random-zebra)

Pull request description:

  Based on:
  - [x] #1554

  This introduces `prevector<N, T>`, a new basic data type which is a fully API compatible drop-in replacement for `std::vector<T>` and uses it for the script, to reduce the considerable memory overhead of vectors in cases where they normally contain a small number of small elements.

  Original tests in Bitcoin showed a reduction in dbcache memory usage by **23%**, and made an initial sync **13%** faster.

  Backported from the following upstream's PRs, with only additional edits in the 2nd layer scripts (`masternode-payments`, `masternode-budget`) and in the unit tests (to address the differences in `insecure_rand`):

  - bitcoin#6914
  - bitcoin#7888
  - bitcoin#8850
  - bitcoin#9349
  - bitcoin#9505
  - bitcoin#9856
  - bitcoin#10534
  - bitcoin#11011
  - bitcoin#14028

  ~~NOTE: Updates to `memusage.h` needed as well, when #1531 is merged.~~

ACKs for top commit:
  furszy:
    Tested ACK a8b965e .
  Fuzzbawls:
    ACK a8b965e

Tree-SHA512: 203b660dd8d9f98780be660dae0ac951766ba438836bd6f0ec921e7749c1a84143f3b920fe49f2cc399f4aa7e763098f78746d3b6ff845bcf913bb13de984bc1
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+3 more reviewers

@dcousens dcousens dcousens approved these changes

@gmaxwell gmaxwell gmaxwell left review comments

@jonasschnelli jonasschnelli jonasschnelli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@sipa @JeremyRubin @theuni @gmaxwell @laanwj @jonasschnelli @dcousens @fanquake