script: actually trigger the optimization in BuildScript #25709
Conversation
The counter is an optimization over calling `ret.empty()`. It was suggested that the compiler would realize `cnt` is only `0` on the first iteration, and not actually emit the check and conditional. This optimization was actually not triggered at all, since we incremented `cnt` at the beginning of the first iteration. Fix it by incrementing at the end instead. This was reported by Github user "Janus".
| @@ -600,6 +599,7 @@ CScript BuildScript(Ts&&... inputs) | |||
| // Otherwise invoke CScript::operator<<. | |||
| ret << input; | |||
| } | |||
| cnt++; | |||
There was a problem hiding this comment.
@darosior did you want to address this comment?
There was a problem hiding this comment.
I was not planning to unless anyone felt strongly otherwise, as it's really a nit..
There was a problem hiding this comment.
If you're interested (for now or later), in addition to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c there is further rationale in https://lefticus.gitbooks.io/cpp-best-practices/content/08-Considering_Performance.html#prefer-i-to-i.
There was a problem hiding this comment.
I haven't yet seen an iterator type where this isn't optimized to the same assembly. Though, if we want this, it might be better enforced with a tidy check than with review comments that invalidate review
There was a problem hiding this comment.
If we don't want this, it should be removed from the developer notes.
There was a problem hiding this comment.
Agree. See also #24846 (comment)
There was a problem hiding this comment.
Choosing ++v; over v++; is generally good advice if you don't want to go into details, but in practically it really matters for performance in case v is a non-trivial, big, type. In this case where it's just an integer it won't make to any difference in the resulting binary code with any compiler created the past 20 years.
|
utACK 00897d0 |
|
review ACK 00897d0 |
…ldScript 00897d0 script: actually trigger the optimization in BuildScript (Antoine Poinsot) Pull request description: The counter is an optimization over calling `ret.empty()`. It was suggested that the compiler would realize `cnt` is only `0` on the first iteration, and not actually emit the check and conditional. This optimization was actually not triggered at all, since we incremented `cnt` at the beginning of the first iteration. Fix it by incrementing at the end instead. This was reported by Github user "Janus". Fixes bitcoin#25682. Note this does *not* change semantics. It only allows the optimization of moving instead of copying on first `CScript` element to actually be reachable. ACKs for top commit: sipa: utACK 00897d0 MarcoFalke: review ACK 00897d0 Tree-SHA512: b575bd444b0cd2fe754ec5f3e2f3f53d2696d5dcebedcace1e38be372c82365e75938dfe185429ed5a83efe1a395e204bfb33efe56c10defc5811eaee50580e3
The counter is an optimization over calling
ret.empty(). It wassuggested that the compiler would realize
cntis only0on the firstiteration, and not actually emit the check and conditional.
This optimization was actually not triggered at all, since we
incremented
cntat the beginning of the first iteration. Fix it byincrementing at the end instead.
This was reported by Github user "Janus".
Fixes #25682. Note this does not change semantics. It only allows the optimization of moving instead of copying on first
CScriptelement to actually be reachable.