Re: Patch for bug 25063 - _pclose should wait for the command processor to terminate and return it's exit status
Borut Razem <borut.razem(a)siol.net> writes:
+static void msvcrt_add_handle(MSVCRT_FILE *fp, HANDLE *hProcess) +{ + LOCK_POPEN; + if (fp2handle == NULL) + { + /* array not allocated yet: allocate it */ + if ((fp2handle = MSVCRT_malloc(sizeof (struct fp2handle_s) * 16)) != NULL) + { + /* write handles to the first slot */ + fp2handle[0].fp = fp; + fp2handle[0].hProcess = hProcess; + + /* zero the remaining space */ + memset(&fp2handle[1], 0, sizeof (struct fp2handle_s) * (16 - 1)); + + /* update the number of slots */ + fp2handle_entries = 16; + } + } + else + { + int i; + + /* search for free or slot with the same fp */ + for (i = 0; i < fp2handle_entries; ++i) + { + if (fp2handle[i].fp == NULL || fp2handle[i].fp == fp) + { + /* found: write handles to the slot */ + fp2handle[i].fp = fp; + fp2handle[i].hProcess = hProcess; + break; + } + } + if (i >= fp2handle_entries) + { + /* no space left: double the array size */ + struct fp2handle_s *newFp2handle; + + if ((newFp2handle = MSVCRT_realloc(fp2handle, sizeof (struct fp2handle_s) * (fp2handle_entries * 2))) != NULL) + { + fp2handle = newFp2handle; + + /* write handles to the first exetended slot */ + fp2handle[fp2handle_entries].fp = fp; + fp2handle[fp2handle_entries].hProcess = hProcess; + + /* zero the remaining extended space */ + memset(&fp2handle[fp2handle_entries + 1], 0, sizeof (struct fp2handle_s) * (fp2handle_entries - 1)); + + /* update the number of slots */ + fp2handle_entries *= 2; + } + } + } + UNLOCK_POPEN; +}
That's an awful lot of code for such a simple thing. You probably want to use an array indexed by file descriptor or something like that. Also please don't add a comment before every line stating what the line does, that should be clear from the code itself. -- Alexandre Julliard julliard(a)winehq.org
On 11/16/2010 01:56 PM, Alexandre Julliard wrote:
Borut Razem<borut.razem(a)siol.net> writes:
+static void msvcrt_add_handle(MSVCRT_FILE *fp, HANDLE *hProcess) +{ + LOCK_POPEN; + if (fp2handle == NULL) + { + /* array not allocated yet: allocate it */ + if ((fp2handle = MSVCRT_malloc(sizeof (struct fp2handle_s) * 16)) != NULL) + { + /* write handles to the first slot */ + fp2handle[0].fp = fp; + fp2handle[0].hProcess = hProcess; + + /* zero the remaining space */ + memset(&fp2handle[1], 0, sizeof (struct fp2handle_s) * (16 - 1)); + + /* update the number of slots */ + fp2handle_entries = 16; + } + } + else + { + int i; + + /* search for free or slot with the same fp */ + for (i = 0; i< fp2handle_entries; ++i) + { + if (fp2handle[i].fp == NULL || fp2handle[i].fp == fp) + { + /* found: write handles to the slot */ + fp2handle[i].fp = fp; + fp2handle[i].hProcess = hProcess; + break; + } + } + if (i>= fp2handle_entries) + { + /* no space left: double the array size */ + struct fp2handle_s *newFp2handle; + + if ((newFp2handle = MSVCRT_realloc(fp2handle, sizeof (struct fp2handle_s) * (fp2handle_entries * 2))) != NULL) + { + fp2handle = newFp2handle; + + /* write handles to the first exetended slot */ + fp2handle[fp2handle_entries].fp = fp; + fp2handle[fp2handle_entries].hProcess = hProcess; + + /* zero the remaining extended space */ + memset(&fp2handle[fp2handle_entries + 1], 0, sizeof (struct fp2handle_s) * (fp2handle_entries - 1)); + + /* update the number of slots */ + fp2handle_entries *= 2; + } + } + } + UNLOCK_POPEN; +}
That's an awful lot of code for such a simple thing. You probably want to use an array indexed by file descriptor or something like that. Also please don't add a comment before every line stating what the line does, that should be clear from the code itself.
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). 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 ;-) If you still think that we should use a static array, just let me know, I'll re-implement it. And let me know what should be the array size. Borut
Borut Razem <borut.razem(a)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.
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. 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). -- Alexandre Julliard julliard(a)winehq.org
On 11/17/2010 11:45 AM, Alexandre Julliard wrote:
Borut Razem<borut.razem(a)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
Borut Razem <borut.razem(a)siol.net> writes:
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's not an issue, if it's unused the cost is minimal. The real problem is that the ioinfo structure can't be changed.
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?
You are confusing the allocation and the lookup. Yes, you need to grow it dynamically. That doesn't mean you need a linear search. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Borut Razem