* 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).