From: Clayton H. <cla...@sp...> - 2003-09-27 06:13:54
|
Hi Gerald, Thanks again for help on the tests! Responses inline. > >3 new test files attached for Streams and FileHandler directories. Committed. =20 >I have a slight concern with CvsStream in that it not only contains a=20 >Stream class but it also inherits from Stream. >CvsStream overrides most of the Stream members and calls the same function=20 >on the contained Stream. However it does not override all of the Stream=20 >members, so if someone was to call one of these it would act on the=20 >inherited Stream and not the contained Stream, which is not what one would=20 >expect. That might be a bad thing. I think that we can minimize this when we start converting the public classes to internal. I have the nant builds working again and just need to get the unit tests I broke back in the good. After that maybe we can start looking at what needs to be made internal. >The easiest solution would be for CvsStream not to inherit from Stream. I=20 >have tried this and everything still compiled, but I don't know if this >will cause any problems for anyone else, or if the inheritance is there for=20 >a good reason? I am not aware of a reason. If it is an easy change to remove the inheritance then maybe we should explore that. It might be a good idea to add a few more tests for different protocols (ext/ssh) first on the off chance that had something to do with the design. Just a thought, no evidence to back that up :-). >One other thing I noticed in the file handlers when both sending and=20 >receiving text files is that they make a physical copy of the file on the=20 >hard disk (to simplify the process of converting linefeeds betwen windows=20 >and *nix). This seems particularly silly if you are using #cvslib under=20 >linux/mono where obviously no translation needs to take place. I see no=20 >reason why this couldn't always be done in-line which would save on the >extra disk activity. Low priority I know compared to the other items that=20 >need doing, but worth doing at some point. I did not think about the line endings, this was one of the things I assumed was being handled magically by the framework :-). I think the temp files have two purposes then, one for the line endings and the other to keep the memory footprint small if the client is bringing down some larger files. Still I agree with you about performance, although it would be a low priority item. How does this sound: before the server sends down a file it sends down a byte count. We do a test to see if this byte count is above a certain threshold (probably on a per system basis, grab the memory installed and only use a reasonable fraction of that) we spool, if not then we use an in memory stream.=20 If that works for you then we can add it to a bug/feature request so we remember it exists. Cheers, Clayton |
From: Gerald E. <gn...@f2...> - 2003-09-27 11:25:49
|
At 22:43 26/09/03 -0700, Clayton Harbour wrote: > >One other thing I noticed in the file handlers when both sending and > >receiving text files is that they make a physical copy of the file on >the > >hard disk (to simplify the process of converting linefeeds betwen >windows > >and *nix). This seems particularly silly if you are using #cvslib >under > >linux/mono where obviously no translation needs to take place. I see >no > >reason why this couldn't always be done in-line which would save on the > > >extra disk activity. Low priority I know compared to the other items >that > >need doing, but worth doing at some point. > >I did not think about the line endings, this was one of the things I >assumed was being handled magically by the framework :-). I think the >temp files have two purposes then, one for the line endings and the >other to keep the memory footprint small if the client is bringing down >some larger files. Alas, the current code allocates a single block of memory and reads the entire file into it before sending it, and similarly when receiving files. So this will need changing to avoid problems supporting large files. >Still I agree with you about performance, although >it would be a low priority item. > >How does this sound: before the server sends down a file it sends down a >byte count. We do a test to see if this byte count is above a certain >threshold (probably on a per system basis, grab the memory installed and >only use a reasonable fraction of that) we spool, if not then we use an >in memory stream. It should just be a matter of using a buffered stream. I'm not sure what buffering the network stream class provides, but it should be possible to provide buffering (to minimise network packets) without the need to spool the file to disk. The fine details can be looked into when someone implements the change. >If that works for you then we can add it to a bug/feature request so we >remember it exists. Yes, both the removal of the need to use a temporary file and the removal of allocating a single block of memory for the entire file need to be added. Makes sense if both of these are addresed together. Gerald. |
From: Clayton H. <cla...@sp...> - 2003-09-27 22:06:08
|
>Alas, the current code allocates a single block of memory and reads the >entire file into it before sending it, and similarly when receiving=20 >files. So this will need changing to avoid problems supporting large files. I looked closer at this and your are right it is just keeping the stream in memory. Probably not a large problem but something that we will need to look at for sure. >It should just be a matter of using a buffered stream. I'm not sure what=20 >buffering the network stream class provides, but it should be possible to=20 >provide buffering (to minimise network packets) without the need to spool=20 >the file to disk. The fine details can be looked into when someone=20 >implements the change. >>If that works for you then we can add it to a bug/feature request so we >>remember it exists. >Yes, both the removal of the need to use a temporary file and the removal=20 >of allocating a single block of memory for the entire file need to be=20 >added. Makes sense if both of these are addresed together. How many hours of effort do you think it would be to put these changes in Gerald? =20 |