On Thu, 2011-09-15 at 17:00 +0200, Jan Zelený wrote:
> > On Tue, 2011-08-30 at 17:54 +0200, Jan Zelený wrote:
> > > Attached patch rewrites almost entire memberof plugin. It heavily
> > > utilizes hash tables instead of lists and arrays and it introduces
> > > concept of reference counting which should heavily optimize all
> > > operations when no loops are present in the user/group tree.
> > >
> > > I tested the patch by running sysdb test suite, all tests passed. I
> > > also attach a document where basic concepts of the plugin are
> > > explained.
> > >
> > > Please note that the patch is functional, although I don't consider
> > > it ready. I just want to get pre-ACK or some comments about the
> > > patch design. I have yet to implement the recompute task, I will
> > > work on that later.
> >
> > Jan,
> > yesterday I an Stephen got together to start a review of the patch. Due
> > to the complexity we tried to go thorugh the code together to make sure
> > we understand what is going on.
> >
> > Also due to the complexity and need to refresh memory we were only be
> > able to go through most of the "add" code only, we were not able to
> > review the update or delete code paths yet.
> >
> > Attached find a diff file with comments in C++ style (so that it is
> > clear they all need to be resolved before the patch can be considered
> > ok, as they show as a big red line in our editors) that you can
> > integrate in your tree and check one by one.
> >
> > There are some questions in there, but in general we noticed a very
> > annoying lack of comments in the code itself. The accompaning PDF is
> > nice and all, but unless the logic is embedded and explained in the
> > code (like it was with the previous code) it will be ablocker and will
> > make review more difficult.
> >
> > That said, so far so good, although we need to more carefully review
> > the refcounting stuff, we got to it late and only marginally reviewed
> > it. Need more time to fully grasp all details.
> >
> > HTH,
> > Simo.
>
> Simo,
> first I'd like to thank you both for the review. I realize it is quite
> difficult, I wouldn't want to do it myself.
>
> I will look at the diff you wrote about ASAP. That said, I already have a
> modification which resolves some pretty big issues which I somehow
> managed to miss at first. It also modifies the full update code path so
> it can be used by the recompute task. I will certainly add some more
> comments, up until now I've been focusing on the code itself. Hopefully
> I'll have the patch complete by the end of the week (i.e. tomorrow) and
> I'll send it. If you wish, we can schedule a phone call to discuss some
> places which are the most difficult to comprehend.
Yes we should set up a call between me you and Stephen next week. so we
can start a through review.
Also one other thing we forgot to mention is that we were wondering if
you made any performance measurements.
Perf. was the main driver for this work, so we would like to see some
numbers to make sure we did actually achieved perf. gains with the new
patchset. It would be pretty lame to switch a very complex system with
another very complex (and largely untested) system and find out we got
little to no gain :-)
Simo.
I did some testing and I was disappointed, the improvement was very small. But
on the other hand, I was just running the sysdb test suite and the main
difference should be seen when using complex rfc2307bis membership structures.
I will set up a testing environment during the next week, hopefully I'll get
some satisfying results.
Jan