refactor: net: avoid duplicate map lookups to `mapLocalHost` #22896

theStack commented at 10:32 PM on September 5, 2021: member

This simple refactoring PR aims to avoid duplicate lookups to mapLocalHost: instead of calling count() (to first find out whether a key is in the map) and then operator[] (to get the value to the passed key, or default-construct one if not found), use either

find() and dereference the returned iterator (for simple lookups), see https://www.cplusplus.com/reference/map/map/find/
emplace() and use the returned <iterator, inserted> pair (for lookups where a new element should be inserted if the key isn't found), see https://www.cplusplus.com/reference/map/map/emplace/

DrahtBot added the label P2P on Sep 5, 2021

DrahtBot added the label Refactoring on Sep 5, 2021

in src/net.cpp:247 in 11632dfaab outdated

 243 | @@ -243,10 +244,10 @@ bool AddLocal(const CService& addr, int nScore)
 244 |  
 245 |      {
 246 |          LOCK(cs_mapLocalHost);
 247 | -        bool fAlready = mapLocalHost.count(addr) > 0;
 248 | -        LocalServiceInfo &info = mapLocalHost[addr];
 249 | -        if (!fAlready || nScore >= info.nScore) {
 250 | -            info.nScore = nScore + (fAlready ? 1 : 0);
 251 | +        auto [it, is_newly_added] = mapLocalHost.insert({addr, LocalServiceInfo()});

MarcoFalke commented at 7:13 AM on September 6, 2021:

Nit: See https://en.cppreference.com/w/cpp/container/map/emplace , if it compiles:

        auto [it, is_newly_added] = mapLocalHost.emplace(addr, LocalServiceInfo());

LarryRuane commented at 7:21 PM on September 6, 2021:

nit, add const

theStack commented at 9:26 PM on September 12, 2021:

Good idea, done.

theStack commented at 4:25 PM on September 14, 2021:

Thanks, done.

in src/net.cpp:250 in 11632dfaab outdated

 249 | -        if (!fAlready || nScore >= info.nScore) {
 250 | -            info.nScore = nScore + (fAlready ? 1 : 0);
 251 | +        auto [it, is_newly_added] = mapLocalHost.insert({addr, LocalServiceInfo()});
 252 | +        LocalServiceInfo &info = it->second;
 253 | +        if (is_newly_added || nScore >= info.nScore) {
 254 | +            info.nScore = nScore + (!is_newly_added ? 1 : 0);

promag commented at 5:04 PM on September 6, 2021:

nit, maybe simplify is_newly_added ? 0 : 1?

theStack commented at 9:26 PM on September 12, 2021:

Thanks, done.

in src/net.cpp:292 in 11632dfaab outdated

 290 | @@ -290,9 +291,9 @@ bool SeenLocal(const CService& addr)
 291 |  {
 292 |      {

promag commented at 5:05 PM on September 6, 2021:

nit, could also ditch these inner scopes.

theStack commented at 9:26 PM on September 12, 2021:

Good idea, done.

promag commented at 5:05 PM on September 6, 2021: member

Code review ACK 11632dfaaba98378dd10905549251590b321452f.

klementtan approved

klementtan commented at 7:09 PM on September 6, 2021: contributor

crAck 11632df

Verified that refactoring count and operator[] to find() and insert() are the only changes.

LarryRuane approved

LarryRuane commented at 7:36 PM on September 6, 2021: contributor

crtACK 11632dfaaba98378dd10905549251590b321452f Ran mainnet and the functions tests. Verified that there are no additional similar changes that could be made in net.cpp (you found them all).

in src/net.cpp:195 in 11632dfaab outdated

 189 | @@ -190,8 +190,9 @@ CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices)
 190 |  static int GetnScore(const CService& addr)
 191 |  {
 192 |      LOCK(cs_mapLocalHost);
 193 | -    if (mapLocalHost.count(addr) == 0) return 0;
 194 | -    return mapLocalHost[addr].nScore;
 195 | +    const auto it = mapLocalHost.find(addr);
 196 | +    if (it == mapLocalHost.end()) return 0;
 197 | +    return it->second.nScore;

jonatack commented at 7:48 PM on September 6, 2021:

-    if (it == mapLocalHost.end()) return 0;
-    return it->second.nScore;
+    return (it == mapLocalHost.end()) ? 0 : it->second.nScore

theStack commented at 9:28 PM on September 12, 2021:

Good idea, done. Though I negated the condition to have the "found" outcome first, which seems more natural to me.

in src/net.cpp:296 in 11632dfaab outdated

 290 | @@ -290,9 +291,9 @@ bool SeenLocal(const CService& addr)
 291 |  {
 292 |      {
 293 |          LOCK(cs_mapLocalHost);
 294 | -        if (mapLocalHost.count(addr) == 0)
 295 | -            return false;
 296 | -        mapLocalHost[addr].nScore++;
 297 | +        const auto it = mapLocalHost.find(addr);
 298 | +        if (it == mapLocalHost.end()) return false;
 299 | +        it->second.nScore++;

jonatack commented at 7:49 PM on September 6, 2021:

prefer prefix operator per developer-notes.md

        ++it->second.nScore;

theStack commented at 9:28 PM on September 12, 2021:

Thanks, done.

jonatack commented at 7:52 PM on September 6, 2021: member

Concept ACK (laptop is chugging away building bitcoind on another branch so these are drive-by untested comments, feel free to ignore)

theStack force-pushed on Sep 12, 2021

theStack commented at 9:30 PM on September 12, 2021: member

Thanks to all reviewers! Force-pushed with all the suggestions proposed by @promag, @LarryRuane and @jonatack.

promag commented at 9:37 PM on September 12, 2021: member

Code review ACK fa75c590f3e26d675d5e5506aba49e159fe20221.

klementtan commented at 7:15 AM on September 13, 2021: contributor

crAck fa75c590f3e26d675d5e5506aba49e159fe20221

in src/net.cpp:293 in fa75c590f3 outdated

 293 | -        if (mapLocalHost.count(addr) == 0)
 294 | -            return false;
 295 | -        mapLocalHost[addr].nScore++;
 296 | +    LOCK(cs_mapLocalHost);
 297 | +    const auto it = mapLocalHost.find(addr);
 298 | +    if (it != mapLocalHost.end()) {

MarcoFalke commented at 7:54 AM on September 13, 2021:

The diff would be smaller by one line if you kept the early return like it was before.

    if (it == mapLocalHost.end()) {

theStack commented at 4:26 PM on September 14, 2021:

Done.

MarcoFalke approved

refactor: net: avoid duplicate map lookups to `mapLocalHost` 330d3aa1a2

theStack force-pushed on Sep 14, 2021

theStack commented at 4:27 PM on September 14, 2021: member

Force-pushed with suggestions of @MarcoFalke (https://github.com/bitcoin/bitcoin/pull/22896#discussion_r702647329, #22896 (review)).

naumenkogs commented at 9:19 AM on September 17, 2021: member

ACK 330d3aa1a2c740dfa31bed3a6ed6b5f88e5426ad

Note, PR description is no longer accurate insert() and use the returned <iterator, inserted> pair (for lookups where a new element should be inserted if the key isn't found), see https://www.cplusplus.com/reference/map/map/insert/

You're using emplace, not insert.

theStack commented at 11:20 AM on September 17, 2021: member

Note, PR description is no longer accurate insert() and use the returned <iterator, inserted> pair (for lookups where a new element should be inserted if the key isn't found), see https://www.cplusplus.com/reference/map/map/insert/

You're using emplace, not insert.

Thanks for your review and the hint, I updated the PR description accordingly.

in src/net.cpp:249 in 330d3aa1a2

 248 | -        if (!fAlready || nScore >= info.nScore) {
 249 | -            info.nScore = nScore + (fAlready ? 1 : 0);
 250 | +        const auto [it, is_newly_added] = mapLocalHost.emplace(addr, LocalServiceInfo());
 251 | +        LocalServiceInfo &info = it->second;
 252 | +        if (is_newly_added || nScore >= info.nScore) {
 253 | +            info.nScore = nScore + (is_newly_added ? 0 : 1);

jonatack commented at 12:07 PM on September 17, 2021:

:stuck_out_tongue:

            info.nScore = nScore + !is_newly_added;

jonatack commented at 12:13 PM on September 17, 2021: member

Code review ACK 330d3aa1a2c740dfa31bed3a6ed6b5f88e5426ad plus rebase to master + debug build

MarcoFalke merged this on Sep 17, 2021

MarcoFalke closed this on Sep 17, 2021

in src/net.cpp:194 in 330d3aa1a2

 189 | @@ -190,8 +190,8 @@ CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices)
 190 |  static int GetnScore(const CService& addr)
 191 |  {
 192 |      LOCK(cs_mapLocalHost);
 193 | -    if (mapLocalHost.count(addr) == 0) return 0;
 194 | -    return mapLocalHost[addr].nScore;
 195 | +    const auto it = mapLocalHost.find(addr);
 196 | +    return (it != mapLocalHost.end()) ? it->second.nScore : 0;

MarcoFalke commented at 12:30 PM on September 17, 2021:

Obviously doesn't matter here, but if there is an else to the if, I prefer to not invert the condition:

     return it == mapLocalHost.end() ? 0 : it->second.nScore;

sidhujag referenced this in commit 67e7eab01d on Sep 19, 2021

theStack deleted the branch on Sep 21, 2021

PastaPastaPasta referenced this in commit cf403b7ca7 on Mar 13, 2022

gades referenced this in commit cae9c6191c on May 16, 2022

DrahtBot locked this on Oct 30, 2022