On 11/17/2010 11:45 AM, Alexandre Julliard wrote:
Borut Razemborut.razem@siol.net writes:
Yes, it looks huge, but we can reduce by a third by removing empty lines and comments (just kidding ;-). But the run-time overhead is minimal: allocation of the array at the first popen call and reallocation if more then 16 childs are popened, which will probably happen very rarely. When the array is allocated, the cost is the same as in case of statically allocated array (OK, one additional "if" for initialization).
The main cost is in the linear search. This doesn't scale.
This is true, but again, normally there are only few childs popened, so the cost is (in my opinion) not so high. OTOH you was complaining about the amount of code: binary search, tree search or hash table implementation would drastically increase the amount and complexity of the code and there would not be much benefit (if any at all) for a small array.
Just FYI: in glibc this is implemented as a linked list of malloced elements, so the overhead is even higher: a linear serach is used and each element have to be allocated & initialized separately, not to mention CPU cache locality problem...
I implemented it as flexible as possible so that there are no limitations about the number of popened childs and no overhead in data space used by statically allocated arrays. In file.c there is a statically allocated array of file descriptors with 2048 elements, which seems a big overhead for popened childs: usually there are only few childs popened per process. OTOH there is a comment in file.c saying: /* FIXME: this should be allocated dynamically */, which is exactly what I did ;-)
The best would be to put it directly in the file descriptors array, but unfortunately there doesn't seem to be any room for this.
I was also thinking abut that. We could add a hProcess member to the ioinfo structure, but this means MSVCRT_MAX_FILES * sizeof(HANDLE) = 2048 * 4 = 8KB additional data space, which will be more then less unused...
That doesn't mean you need a static array, you can still allocate a separate array dynamically (and then of course you don't need 2048, only the highest file descriptor in use).
The problem is that "the highest file descriptor in use" (actually the number of popen calls during the process life) is not konwn in advance, that's why I implemented it as "dynamically growing array". Or I'm missing something?
P.S.: One small (if any at all) improvement I can see is to use file descriptors instead of file pointers in the array. This would save some memory, since a "short int" could be used instead of "MSVCRT_FILE *", which theoretically saves 2 bytes per entry on 32-bit machine (6 bytes on 64-bit). The actual saving depends on struct alignment. But this is not related to any of problems you exposed...
Borut