openssh unix dev - Aug 2007 - Channel Patch

I've updated the channel patch including some suggestions. The main 
difference is that I eliminated the channels[] array entirely (and the 
attendant code to create the array). I did not, however, include linked 
list pointers in the Channel struct. Mostly because its easier for me to 
work with at this time. I expect I'll do it in the next iteration though.

Removing the channels array does mean that channel_by_id must iterate 
through a linked list rather than doing an index lookup. I don't think 
this will have much impact on performance in the majority of cases. On 
the plus side, I think I can eliminate a lot of the NULL conditionals. I 
haven't yet though.

Anyway, if anyone is interested the patches are attached. Any comments, 
observations, or such not is greatly appreciated.

At this time this should still be viewed as development code. 
Preliminary tests indicated that there were no performance hits and 
there were some hints that in some situations it will provide a 
performance improvement.

Chris Rapier

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: channels.h.diff
Url:
http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20070803/1628a9e3/attachment.ksh
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: channels.c.diff
Url:
http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20070803/1628a9e3/attachment-0001.ksh

On Fri, 3 Aug 2007, Chris Rapier wrote:
> I've updated the channel patch including some suggestions. The main
> difference is that I eliminated the channels[] array entirely (and
> the attendant code to create the array). I did not, however, include
> linked list pointers in the Channel struct. Mostly because its easier
> for me to work with at this time. I expect I'll do it in the next
> iteration though.
>
> Removing the channels array does mean that channel_by_id must iterate
> through a linked list rather than doing an index lookup. I don't think
> this will have much impact on performance in the majority of cases. On
> the plus side, I think I can eliminate a lot of the NULL conditionals.
> I haven't yet though.
>
> Anyway, if anyone is interested the patches are attached. Any
> comments, observations, or such not is greatly appreciated.
>
> At this time this should still be viewed as development code.
> Preliminary tests indicated that there were no performance hits and
> there were some hints that in some situations it will provide a
> performance improvement.
Before you go any further with this diff, please understand that it 
almost no chance of being incorporated unless it follows the 
style of the existing code[1].

We also prefer to use the queue[2] macros instead of hand-rolled
linked list implementations, though I'm not sure that a linked list is
the right data structure (remember you need to support by-id lookups
too), whether you need to change the data structure at all or whether
traversing an array of a few dozen elements is worth optimising.

It seems you could avoid most of the channel array scans by simply
tracking the number of channels actually used and stopping the loop
after seeing that many open channels (this would work very well unless
the entries were fragmented across the array). If you wanted to be
tricky, you could add a precomputed skip step to the channels array that
would point to the next allocated channel, allowing the loop to skip
past unused channels.

-d

[1] http://www.openbsd.org/cgi-bin/man.cgi?query=style&sektion=9
[2] http://www.openbsd.org/cgi-bin/man.cgi?query=queue&sektion=3