-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Optimize lookups in NodeDB #9480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR optimizes NodeDB lookups by maintaining the meshNodes vector sorted by NodeNum to enable O(log n) binary search instead of O(n) linear search. The existing display-order sorting (own node → favorites → last_heard) is moved to a separate lazy-built pointer vector that's only rebuilt when needed for display access.
Changes:
- Refactored NodeDB to use binary search for node lookups by sorting meshNodes by NodeNum
- Separated display ordering into displayNodes pointer vector with lazy rebuild on access
- Added favoriteRouterLastBytes cache to optimize Router's shouldDecrementHopLimit check
- Made meshNodes private and added appropriate accessor methods
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mesh/NodeDB.h | Added private meshNodes field, displayNodes vector, dirty flag, and new helper methods (rebuildDisplayOrder, sortByNodeNum, rebuildFavoriteRouterIndex) |
| src/mesh/NodeDB.cpp | Implemented binary search in getMeshNode, insertion/removal maintaining sorted order, display order rebuilding, favorite router caching, and updated resetNodes/removeNodeByNum/cleanupMeshDB |
| src/modules/CannedMessageModule.cpp | Simplified lookup by using getMeshNode directly instead of manual iteration |
| src/modules/AdminModule.cpp | Replaced direct node modification with set_favorite method |
| src/mesh/Router.cpp | Optimized shouldDecrementHopLimit using cached favoriteRouterLastBytes |
| src/modules/NodeInfoModule.cpp | Updated to use getNumMeshNodes() instead of direct meshNodes access |
| src/mesh/PhoneAPI.cpp | Changed STATE_SEND_OWN_NODEINFO to use getMeshNode instead of readNextMeshNode |
| src/graphics/niche/InkHUD/Applets/User/Heard/HeardApplet.cpp | Updated to use readNextMeshNode and const correctness |
This is because that is how we leverage the node list, when looking at the node list on a service, that is the order they are displayed, so sorting against this order makes the most sense. We just filter based upon having a location for some of the GPS frame. While ordering the order every 5 seconds maybe can be backed off a bit (maybe just sort on change?) I don't understand the perform game of going to node number order to not use the data in that fashion. |
|
Re: Xaositek The display order (own node > favorites > last heard) is fully preserved. The displayNodes pointer vector still provides exactly that, and it's what getMeshNodeByIndex() returns. Nothing changes about how the UI sees or iterates nodes. The nodeNum ordering applies to meshNodes, the internal structure used for lookups by node ID (e.g., finding a sender's node info when a packet arrives). That's currently a linear scan, and nodeNum sorted order gives us binary search instead. So really it's a separation of concerns: display code sees the same ordering it always has, internal lookups get faster. This is a relatively minor optimization in the realm of a largely IO bound mesh node. However, it can be made with little impact to the existing code structure and with full backward compatibility for the existing network. As long as the complexity or code size doesn't increase too much, small wins are still wins. The less time a node can spend doing CPU bound tasks, the more time it can spend sleeping and conserving battery. On the display sort throttle, it could potentially be eliminated completely since the node list is now marked dirty when it is updated. It is probably best to keep the throttle/holddown since a mesh with lots of active nodes could be updated quite frequently and the end user probably doesn't care about or actually desire the list sorted in real time. |
jp-bennett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything I immediately hate. I do remember introducing a very mysterious crash the last time I worked with this sorting code, so we want to test this pretty thoroughly before shipping it. I know this is a pain, but maybe wait for the 2.8 releases?
This pull request modifies NodeDB to keep the meshNodes vector sorted by nodeNum so that it can be searched using a binary search O(log n).
The existing code prioritizes sorting for display: own node, favorites, last heard.
This display sorting forces a linear search O(n), which is usually not noticeable for small numbers of active nodes. As the number of nodes increases, this overhead can become significant for low power embedded boards.
In the existing code, the nodes are resorted every 5 seconds, regardless of if a user is viewing the node list or not. This pull request keeps a separate pointer vector sorted by the display order. The new vector is resorted only as needed for display, and then it is only sorted when something has changed, or every 5 seconds. There is no overhead for resorting if the display access methods are not called.
This pull request also makes meshNodes a private field and updates relevant accesses.
🤝 Attestations