After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 763499 - [review] refactor th/lldp [th/lldp-bgo763499]
[review] refactor th/lldp [th/lldp-bgo763499]
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-03-11 15:17 UTC by Thomas Haller
Modified: 2016-03-17 14:18 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Thomas Haller 2016-03-11 15:17:37 UTC
- Cache the GVariant per LldpNeighbor instance.

- On event from sd-lldp, only update the changed neighbor instead of processing 
  all.
Comment 1 Dan Williams 2016-03-12 14:51:17 UTC
> lldp: refactor processing all lldp-neighbors

Maybe instead of creating a new hash table for the prune list and accessing it a lot, what if instead NMLldpListenerPrivate had a "guint32 neighbor_stamp;" that starts at zero.

Then each call to process_lldp_neighbors() you increment the neighbor_stamp.  At the beginning of the neighbors loop you "num_to_remove = g_hash_table_size(priv->lldp_neighbors)".  Each time you find a neigh_old (equal or unequal) you decrement that because the neighbor won't be removed or it'll get re-added.  Each time you add or keep a neighbor you increment "num_added" and set the neighbor's stamp to priv->neighbor_stamp.  Then the capacity check becomes "if (num_added - num_to_remove > MAX_NEIGHBORS)".

At the end walk priv->lldp_neighbors and compare each neighbor's stamp against priv->neighbor_stamp and if it is less g_hash_table_iter_remove() that one.

Just a thought; then we only use plain arithmetic, don't allocate a new hash table every time, and don't keep searching the hash.

> lldp: factor our lldp_neighbor_to_variant()

our -> out

> lldp: process one neighbor at a time

Can we get process_lldp_neighbors() to call process_lldp_neighbor()?  They use almost exactly the same code.
Comment 2 Thomas Haller 2016-03-15 12:18:15 UTC
(In reply to Dan Williams from comment #1)
> > lldp: refactor processing all lldp-neighbors
>
> Just a thought; then we only use plain arithmetic, don't allocate a new hash
> table every time, and don't keep searching the hash.
> 
> > lldp: process one neighbor at a time
> 
> Can we get process_lldp_neighbors() to call process_lldp_neighbor()?  They
> use almost exactly the same code.

I added another patch "lldp: drop process_lldp_neighbors()" to get rid of process_lldp_neighbors() entirely.


Repushed.
Comment 3 Beniamino Galvani 2016-03-16 13:12:13 UTC
LGTM