script: actually trigger the optimization in BuildScript #25709

1. 
   darosior commented at 11:10 AM on July 26, 2022: member

   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 #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.

2. script: actually trigger the optimization in BuildScript

   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".

   00897d0677
3. DrahtBot added the label Consensus on Jul 26, 2022
4. in src/script/script.h:602 in 00897d0677

    598 | @@ -600,6 +599,7 @@ CScript BuildScript(Ts&&... inputs)
    599 |              // Otherwise invoke CScript::operator<<.
    600 |              ret << input;
    601 |          }
    602 | +        cnt++;
   

   ---

   ---

   

   luke-jr commented at 11:56 PM on July 26, 2022:

           ++cnt;
   

   ---

   

   fanquake commented at 12:36 PM on July 30, 2022:

   @darosior did you want to address this comment?

   ---

   

   darosior commented at 2:57 PM on July 30, 2022:

   I was not planning to unless anyone felt strongly otherwise, as it's really a nit..

   ---

   

   jonatack commented at 3:24 PM on July 30, 2022:

   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.

   ---

   

   MarcoFalke commented at 3:46 PM on July 30, 2022:

   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

   ---

   

   jonatack commented at 3:58 PM on July 30, 2022:

   If we don't want this, it should be removed from the developer notes.

   ---

   

   MarcoFalke commented at 4:05 PM on July 30, 2022:

   Agree. See also #24846 (comment)

   ---

   

   sipa commented at 4:14 PM on July 30, 2022:

   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.

5. 
   sipa commented at 7:37 PM on July 27, 2022: member

   utACK 00897d0677c2cb6609b90d52b89907c8b50d6de0

6. 
   MarcoFalke commented at 7:47 PM on July 27, 2022: member

   review ACK 00897d0677c2cb6609b90d52b89907c8b50d6de0

7. MarcoFalke removed the label Consensus on Jul 27, 2022
8. MarcoFalke added the label Refactoring on Jul 27, 2022
9. MarcoFalke added this to the milestone 24.0 on Jul 27, 2022
10. MarcoFalke merged this on Jul 30, 2022
11. MarcoFalke closed this on Jul 30, 2022

    ---

12. sidhujag referenced this in commit b53c7fa0a7 on Aug 1, 2022
13. bitcoin locked this on Jul 30, 2023

Contributors

darosiorluke-jrfanquakejonatackMarcoFalkesipa

Milestone
24.0