Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip

Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip

[Jon Turney]

> -		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

Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip

[Jon Turney]

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?

Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip

[Achim Gratz]

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

Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip

[Jon Turney]

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.

Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip

[Jon Turney]

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! :)

Re: [PATCH cygport] lib/src_postinst.cygpart: parallelize __prepstrip

[Achim Gratz]

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