* Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip @ 2023-03-26 18:12 Jon Turney 2023-03-27 18:43 ` Jon Turney 0 siblings, 1 reply; 6+ messages in thread From: Jon Turney @ 2023-03-26 18:12 UTC (permalink / raw) To: cygwin-apps, Achim Gratz > - usr/lib/gcc/*/lib*|usr/lib/gcc/*/*.o) > + usr/lib/gcc/*/*.o) Why this change? > + local nproc=$(nproc) This limit should probably be taken from the --jobs command line parameter, if specified ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip 2023-03-26 18:12 [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip Jon Turney @ 2023-03-27 18:43 ` Jon Turney 2023-03-30 19:49 ` Achim Gratz 0 siblings, 1 reply; 6+ messages in thread From: Jon Turney @ 2023-03-27 18:43 UTC (permalink / raw) To: cygwin-apps, Achim Gratz On 26/03/2023 19:12, Jon Turney via Cygwin-apps wrote: >> - usr/lib/gcc/*/lib*|usr/lib/gcc/*/*.o) >> + usr/lib/gcc/*/*.o) > > Why this change? > >> + local nproc=$(nproc) > > This limit should probably be taken from the --jobs command line > parameter, if specified Looking at this a bit more, a couple of perhaps more serious problems: * The parallel invocations of __prepstrip_one are all appending to ${T}/.dbgsrc.out I don't see what makes that safe against interleaving of the output. It's probably possible to have each instance write to a separate file and collect them together in __prepdebugsrc * This patch causes several failures in the testsuite, e.g. with autotools/c testcase. On a brief attempt at debugging, it this looks like it's due to not waiting for all the __prepstrip_one to complete before moving on, but I think the final wait should prevent that, so idk. I'm not clear that invoking 'jobs', is actually doing anything, if job-control is turned off in a non-interactive shell? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip 2023-03-27 18:43 ` Jon Turney @ 2023-03-30 19:49 ` Achim Gratz 2023-03-30 20:34 ` Jon Turney 0 siblings, 1 reply; 6+ messages in thread From: Achim Gratz @ 2023-03-30 19:49 UTC (permalink / raw) To: cygwin-apps Jon Turney via Cygwin-apps writes: > On 26/03/2023 19:12, Jon Turney via Cygwin-apps wrote: >>> - usr/lib/gcc/*/lib*|usr/lib/gcc/*/*.o) >>> + usr/lib/gcc/*/*.o) >> Why this change? It looks like a mistake that I didn't catch. >> >>> + local nproc=$(nproc) >> This limit should probably be taken from the --jobs command line >> parameter, if specified Yes, although one could argue that it should actually oversubscribe since the CPU load per process is expected to be significantly less than 100% per process. > Looking at this a bit more, a couple of perhaps more serious problems: > > * The parallel invocations of __prepstrip_one are all appending to > ${T}/.dbgsrc.out > > I don't see what makes that safe against interleaving of the output. Line-buffering and the line being shorter than the buffer should. > It's probably possible to have each instance write to a separate file > and collect them together in __prepdebugsrc Nah, if you insist on making it _really_ safe regardless of line lenght and buffer size vagaries I'll do file locking on the output file. > * This patch causes several failures in the testsuite, e.g. with > autotools/c testcase. Which? > On a brief attempt at debugging, it this looks like it's due to not > waiting for all the __prepstrip_one to complete before moving on, but > I think the final wait should prevent that, so idk. I've seen an indication that the final wait doesn't work, but that is fixable by a sleep apparently. Did You see that the process number limiting doesn't work? > I'm not clear that invoking 'jobs', is actually doing anything, if > job-control is turned off in a non-interactive shell? No, "jobs" shouldn't do anything, but wait should still work I think (the manpage talks about jobs, but it really means "children"). But then again, a bit of googling tells me that the bashism "wait -n" actually needs job control to be enabled, natch. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2: http://Synth.Stromeko.net/Downloads.html#WaldorfSDada ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip 2023-03-30 19:49 ` Achim Gratz @ 2023-03-30 20:34 ` Jon Turney 2023-04-02 17:39 ` Jon Turney 0 siblings, 1 reply; 6+ messages in thread From: Jon Turney @ 2023-03-30 20:34 UTC (permalink / raw) To: Achim Gratz, cygwin-apps On 30/03/2023 20:49, Achim Gratz via Cygwin-apps wrote: > Jon Turney via Cygwin-apps writes: >> On 26/03/2023 19:12, Jon Turney via Cygwin-apps wrote: >>>> - usr/lib/gcc/*/lib*|usr/lib/gcc/*/*.o) >>>> + usr/lib/gcc/*/*.o) >>> Why this change? > > It looks like a mistake that I didn't catch. > >>> >>>> + local nproc=$(nproc) >>> This limit should probably be taken from the --jobs command line >>> parameter, if specified > > Yes, although one could argue that it should actually oversubscribe > since the CPU load per process is expected to be significantly less than > 100% per process. > >> Looking at this a bit more, a couple of perhaps more serious problems: >> >> * The parallel invocations of __prepstrip_one are all appending to >> ${T}/.dbgsrc.out >> >> I don't see what makes that safe against interleaving of the output. > > Line-buffering and the line being shorter than the buffer should. and what causes line-buffering be on? >> It's probably possible to have each instance write to a separate file >> and collect them together in __prepdebugsrc > > Nah, if you insist on making it _really_ safe regardless of line lenght > and buffer size vagaries I'll do file locking on the output file. If you lock the file while objdump is running, you've basically serialized this again... >> * This patch causes several failures in the testsuite, e.g. with >> autotools/c testcase. > > Which? The full list of failing tests is: 21/54 autotools/c FAIL 63.16s exit status 1 22/54 autotools/gnome FAIL 170.49s exit status 1 23/54 autotools/gtkmm FAIL 177.54s exit status 1 24/54 autotools/mate FAIL 131.24s exit status 1 25/54 autotools/xfce FAIL 85.64s exit status 1 26/54 cmake/c FAIL 13.75s exit status 1 28/54 cmake/qt5 FAIL 94.44s exit status 1 37/54 lua/all FAIL 10.92s exit status 1 38/54 httpd/apxs FAIL 31.98s exit status 1 44/54 perl/Module-Build FAIL 16.09s exit status 1 46/54 php/pecl FAIL 48.13s exit status 1 49/54 qmake/qt5 FAIL 46.84s exit status 1 > >> On a brief attempt at debugging, it this looks like it's due to not >> waiting for all the __prepstrip_one to complete before moving on, but >> I think the final wait should prevent that, so idk. > > I've seen an indication that the final wait doesn't work, but that is > fixable by a sleep apparently. Did You see that the process number > limiting doesn't work? No, but I haven't been trying to test it. >> I'm not clear that invoking 'jobs', is actually doing anything, if >> job-control is turned off in a non-interactive shell? > > No, "jobs" shouldn't do anything, but wait should still work I think > (the manpage talks about jobs, but it really means "children"). But > then again, a bit of googling tells me that the bashism "wait -n" > actually needs job control to be enabled, natch. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip 2023-03-30 20:34 ` Jon Turney @ 2023-04-02 17:39 ` Jon Turney 2023-04-02 19:28 ` Achim Gratz 0 siblings, 1 reply; 6+ messages in thread From: Jon Turney @ 2023-04-02 17:39 UTC (permalink / raw) To: cygwin-apps, Achim Gratz On 30/03/2023 21:34, Jon Turney via Cygwin-apps wrote: > On 30/03/2023 20:49, Achim Gratz via Cygwin-apps wrote: >> Jon Turney via Cygwin-apps writes: >>> On 26/03/2023 19:12, Jon Turney via Cygwin-apps wrote: > Exchange the while loop using an iffy read construct to a for loop using a temporary file. I think this change from zero-delimited to whitespace means this will now fail to handle any filenames containing whitespace correctly? This commentary doesn't clearly identify what is wrong with the usage of read here. > avoid filename collisions by using an > SHA256 hash of the full file name. I think there is already a perfectly good, filesystem safe, computationally cheap unique identifier for each filename, which is it's ordinal number in the list of filenames we are examining. 'wait -f' seems to be new in bash 5.0. I assume this fails horribly on earlier bash versions. I'm ok with requiring that, but maybe we should check the bash version? On the plus side, the testsuite passes! :) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip 2023-04-02 17:39 ` Jon Turney @ 2023-04-02 19:28 ` Achim Gratz 0 siblings, 0 replies; 6+ messages in thread From: Achim Gratz @ 2023-04-02 19:28 UTC (permalink / raw) To: cygwin-apps Jon Turney via Cygwin-apps writes: >> Exchange the while loop using an iffy read construct to a for loop using a temporary file. > > I think this change from zero-delimited to whitespace means this will > now fail to handle any filenames containing whitespace correctly? Yes, sorry. I thought whitespace in filenames wasn't working anyway, but at least here it was done correctly. > This commentary doesn't clearly identify what is wrong with the usage > of read here. The read itself was OK, piping the data from find into the read wasn't. I've replaced this with a process substitution and thus reinstated the whitespace protection without getting into subshell trouble. >> avoid filename collisions by using an >> SHA256 hash of the full file name. > > I think there is already a perfectly good, filesystem safe, > computationally cheap unique identifier for each filename, which is > it's ordinal number in the list of filenames we are examining. I've implemented a counter now. However I don't see the hashing of a filename as onerous when Git does that much more often and on much larger data. > 'wait -f' seems to be new in bash 5.0. I assume this fails horribly > on earlier bash versions. I'm ok with requiring that, but maybe we > should check the bash version? It should indeed be possible to drop the -f as long as job control is not enabled if I understand the manual correctly after re-reading it several times. I've done that and it looks like things still work. > On the plus side, the testsuite passes! :) Oh goody! Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ SD adaptation for Waldorf rackAttack V1.04R1: http://Synth.Stromeko.net/Downloads.html#WaldorfSDada ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-02 19:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-26 18:12 [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip Jon Turney 2023-03-27 18:43 ` Jon Turney 2023-03-30 19:49 ` Achim Gratz 2023-03-30 20:34 ` Jon Turney 2023-04-02 17:39 ` Jon Turney 2023-04-02 19:28 ` Achim Gratz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).