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

theStack · 2021-09-05T22:32:07Z

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/

promag

Code review ACK 11632df.

src/net.cpp

klementtan

crAck 11632df

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

LarryRuane

crtACK 11632df
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).

src/net.cpp

jonatack

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

src/net.cpp

theStack · 2021-09-12T21:30:56Z

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

promag

Code review ACK fa75c59.

klementtan · 2021-09-13T07:15:54Z

crAck fa75c59

src/net.cpp

theStack · 2021-09-14T16:27:04Z

Force-pushed with suggestions of @MarcoFalke (#22896 (comment), #22896 (comment)).

naumenkogs · 2021-09-17T09:19:11Z

ACK 330d3aa

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 · 2021-09-17T11:20:31Z

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.

jonatack

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

jonatack · 2021-09-17T12:07:34Z

src/net.cpp

+        const auto [it, is_newly_added] = mapLocalHost.emplace(addr, LocalServiceInfo());
+        LocalServiceInfo &info = it->second;
+        if (is_newly_added || nScore >= info.nScore) {
+            info.nScore = nScore + (is_newly_added ? 0 : 1);


😛

Suggested change

info.nScore = nScore + (is_newly_added ? 0 : 1);

info.nScore = nScore + !is_newly_added;

maflcko · 2021-09-17T12:30:34Z

src/net.cpp

-    if (mapLocalHost.count(addr) == 0) return 0;
-    return mapLocalHost[addr].nScore;
+    const auto it = mapLocalHost.find(addr);
+    return (it != mapLocalHost.end()) ? it->second.nScore : 0;


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;

…apLocalHost` 330d3aa refactor: net: avoid duplicate map lookups to `mapLocalHost` (Sebastian Falbesoner) Pull request description: 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/ ACKs for top commit: naumenkogs: ACK 330d3aa jonatack: Code review ACK 330d3aa plus rebase to master + debug build Tree-SHA512: d13da6a927ff561eee8ac6b093bf3586dfe31d6c94173a5a6d8f3698e0ee224fb394d3635155d5141c165da59d2c2c37260122eb4f2e8bcda3e8a29b901d213e

DrahtBot added P2P Refactoring labels Sep 5, 2021

promag reviewed Sep 6, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

klementtan approved these changes Sep 6, 2021

View reviewed changes

LarryRuane approved these changes Sep 6, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

jonatack reviewed Sep 6, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

theStack force-pushed the 202109-refactor-net-avoid_duplicate_mapLocalHost_lookups branch from 11632df to fa75c59 Compare September 12, 2021 21:24

promag reviewed Sep 12, 2021

View reviewed changes

maflcko approved these changes Sep 13, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

refactor: net: avoid duplicate map lookups to mapLocalHost

330d3aa

theStack force-pushed the 202109-refactor-net-avoid_duplicate_mapLocalHost_lookups branch from fa75c59 to 330d3aa Compare September 14, 2021 16:25

jonatack reviewed Sep 17, 2021

View reviewed changes

maflcko merged commit e69cbac into bitcoin:master Sep 17, 2021

maflcko reviewed Sep 17, 2021

View reviewed changes

theStack deleted the 202109-refactor-net-avoid_duplicate_mapLocalHost_lookups branch September 21, 2021 13:37

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theStack commented Sep 5, 2021 •

edited

Loading

promag left a comment

klementtan left a comment

LarryRuane left a comment

jonatack left a comment

theStack commented Sep 12, 2021

promag left a comment

klementtan commented Sep 13, 2021

theStack commented Sep 14, 2021

naumenkogs commented Sep 17, 2021

theStack commented Sep 17, 2021

jonatack left a comment

jonatack Sep 17, 2021

maflcko Sep 17, 2021

	info.nScore = nScore + (is_newly_added ? 0 : 1);
	info.nScore = nScore + !is_newly_added;

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

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

Conversation

theStack commented Sep 5, 2021 • edited Loading

promag left a comment

Choose a reason for hiding this comment

klementtan left a comment

Choose a reason for hiding this comment

LarryRuane left a comment

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

theStack commented Sep 12, 2021

promag left a comment

Choose a reason for hiding this comment

klementtan commented Sep 13, 2021

theStack commented Sep 14, 2021

naumenkogs commented Sep 17, 2021

theStack commented Sep 17, 2021

jonatack left a comment

Choose a reason for hiding this comment

jonatack Sep 17, 2021

Choose a reason for hiding this comment

maflcko Sep 17, 2021

Choose a reason for hiding this comment

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

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

theStack commented Sep 5, 2021 •

edited

Loading