Patch for bug 25063 - _pclose should wait for the command processor to terminate and return it's exit status

[Borut Razem]

On 11/17/2010 11:45 AM, Alexandre Julliard wrote:
> Borut Razem<borut.razem at 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