refactor: rpc: Remove vector copy from listtransactions #17746

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-12-listtransactions changing 1 files +4 −17

promag commented at 3:44 PM on December 14, 2019: member
Current approach
- copy accumulated ret vector to arrTmp
- drop unnecessary elements from arrTmp
- reverse arrTmp
- clear ret
- copy arrTmp to the ret
New approach
- create a vector from the accumulated ret with just the necessary elements already reversed
- copy it to the result
This PR doesn't change behavior.
fanquake added the label RPC/REST/ZMQ on Dec 14, 2019
in src/wallet/rpcwallet.cpp:1496 in 1f448777b7 outdated
```
1508 | -    ret.push_backV(arrTmp);
1509 | -
1510 | -    return ret;
1511 | +    const std::vector<UniValue>& txs = ret.getValues();
1512 | +    UniValue result{UniValue::VARR};
1513 | +    result.push_backV({ txs.rend() - nFrom - nCount, txs.rend() - nFrom });
```
jonatack commented at 5:40 PM on December 14, 2019:

This might be more explicit and has a simpler push_backV

+ const std::vector<UniValue, std::allocator<UniValue> >::const_reverse_iterator txs_end = txs.rend() - nFrom; UniValue result{UniValue::VARR}; - result.push_backV({ txs.rend() - nFrom - nCount, txs.rend() - nFrom }); + result.push_backV({txs_end - nCount, txs_end});
promag commented at 3:17 PM on December 18, 2019:

I'll leave the suggestion open for others to weight in.

ryanofsky commented at 6:43 PM on February 6, 2020:

I'll leave the suggestion open for others to weight in.

Just one opinion, but I prefer the current version without the extra variable

ryanofsky commented at 6:44 PM on February 6, 2020:

In commit "rpc: Remove vector copy from listtransactions" (1f448777b76dfddf0e243f9bbf5dd08fea34f386)

Maybe consider keeping the "// Return oldest to newest" comment here. Helps explain this line at a higher level, and it pairs together with the earlier "// ret is newest to oldest" comment in the code.

jonatack commented at 7:01 PM on February 6, 2020:

FWIW kudos on @promag's response to leave it open for others to weigh in -- I noted it as a good example to follow.
jonatack commented at 5:48 PM on December 14, 2019: member

ACK 1f44877 on simplifying this code from 2012 (e3bc569)
paymog commented at 8:17 AM on December 15, 2019: none

ACK 1f448777b76dfddf0e243f9bbf5dd08fea34f386
promag commented at 3:22 PM on December 18, 2019: member

Going to profile but for big wallets this probably improves when the last page is computed.
kristapsk approved
kristapsk commented at 7:16 PM on December 18, 2019: contributor

ACK 1f448777b76dfddf0e243f9bbf5dd08fea34f386. Benchmarked with one wallet with ~32K tx history, didn't notice big difference in performance, but new code is less LOCs, so IMO better.
promag commented at 7:25 PM on December 18, 2019: member

@kristapsk thanks! Have you benchmarked with skip=32k?
laanwj commented at 12:49 PM on December 19, 2019: member

Concept ACK, good to simplify this code, but does need careful review that it still does the same under all circumstances
jonasschnelli commented at 11:45 PM on December 22, 2019: contributor

utACK 1f448777b76dfddf0e243f9bbf5dd08fea34f386
emilengler commented at 7:03 PM on February 2, 2020: contributor

tACK 1f44877, much more elegant way. I tested it and the results (in terms of return value) are the same, good work :+1:
promag requested review from ryanofsky on Feb 6, 2020
ryanofsky approved
ryanofsky commented at 6:53 PM on February 6, 2020: member

Code review ACK 1f448777b76dfddf0e243f9bbf5dd08fea34f386
ryanofsky added the label Refactoring on Feb 6, 2020
ryanofsky commented at 6:55 PM on February 6, 2020: member

Would be good to use "refactor" tag in PR / commit titles, and explicitly say no behavior is changing
laanwj commented at 3:32 PM on February 13, 2020: member

This seems so close to merge. (not sure it's worth invalidating existing ACKs for a comment / commit message change, but will leave it to @promag )
laanwj added the label Waiting for author on Feb 13, 2020
promag renamed this:
~~rpc: Remove vector copy from listtransactions~~
refactor: rpc: Remove vector copy from listtransactions
on Feb 13, 2020
refactor: rpc: Remove vector copy from listtransactions
No change in behavior.
25bc17fceb
promag force-pushed on Feb 13, 2020
promag commented at 3:47 PM on February 13, 2020: member

Incorporated @ryanofsky suggestions, should be easy to re-ack since only change is adding back a comment.
ryanofsky approved
ryanofsky commented at 4:39 PM on February 13, 2020: member

Code review ACK 25bc17fceb08ee9625c5e09e2579117ec6f7a1c5. Just comment and commit message tweaks since last review
laanwj referenced this in commit 470664f2b7 on Feb 13, 2020
laanwj merged this on Feb 13, 2020
laanwj closed this on Feb 13, 2020
promag deleted the branch on Feb 13, 2020
ryanofsky removed the label Waiting for author on Mar 13, 2020
deadalnix referenced this in commit 848640a7a4 on Jul 14, 2020
PastaPastaPasta referenced this in commit 1136aef5c3 on Jun 27, 2021
PastaPastaPasta referenced this in commit 88ac7d0624 on Jun 28, 2021
PastaPastaPasta referenced this in commit dfe497c79f on Jun 29, 2021
PastaPastaPasta referenced this in commit 6f4088be36 on Jul 1, 2021
PastaPastaPasta referenced this in commit 8615b09508 on Jul 1, 2021
PastaPastaPasta referenced this in commit 8965e76e0a on Jul 14, 2021
PastaPastaPasta referenced this in commit 12b73f7df0 on Jul 14, 2021
PastaPastaPasta referenced this in commit 1555ee3a6c on Jul 15, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Refactoring RPC/REST/ZMQ