On Mon, Aug 29, 2022 at 05:36:49PM +0200, Theo Buehler wrote:
> On Mon, Aug 29, 2022 at 05:05:59PM +0200, Claudio Jeker wrote:
> > The hash table for communities is not great. Instead of implementing
> > dynamic hash resize use a RB tree. Also drop the hash calculation and
> > just use memcmp() for now. My non scientific test seems to indicate that
> > the overhead of SipHash is about the same as the memcmp().
>
> Generally looks good. Two comments.
>
> > +static inline int
> > +communities_compare(struct rde_community *a, struct rde_community *b)
> > {
> > - uint32_t hs, i;
> > + if (a->nentries != b->nentries)
> > + return a->nentries - b->nentries;
>
> I dislike comparison by subtraction, but I guess it's fine since nentries
> should never be negative. Should this check and fatal?
I think I copied this from existing code but I can change it to the double
if we use in other places.
> > + if (a->flags != b->flags)
> > + return a->flags - b->flags;
>
> [...]
Here as well
> > struct rde_community *
> > communities_link(struct rde_community *comm)
> > {
> > struct rde_community *n;
> > - struct commhead *head;
> >
> > if ((n = malloc(sizeof(*n))) == NULL)
> > fatal(__func__);
> > -
> > communities_copy(n, comm);
> >
> > - head = communities_hash(n);
> > - LIST_INSERT_HEAD(head, n, entry);
> > + RB_INSERT(comm_tree, &commtable, n);
>
> I would prefer if this insertion was checked.
Since we did not check in the original code either I decided to YOLO it
but maybe an extra check makes sense.
> > n->refcnt = 1; /* initial reference by the cache */
> >
> > rdemem.comm_size += n->size;
> > @@ -886,7 +817,7 @@ communities_unlink(struct rde_community
> > if (comm->refcnt != 1)
> > fatalx("%s: unlinking still referenced communities", __func__);
> >
> > - LIST_REMOVE(comm, entry);
> > + RB_REMOVE(comm_tree, &commtable, comm);
> >
> > rdemem.comm_size -= comm->size;
> > rdemem.comm_nmemb -= comm->nentries;
> >
>
--
:wq Claudio
