Smalltalk › Squeak › Squeak - Dev

Sound playing can break in odd manner

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

13 messages Options

timrowledge

Sound playing can break in odd manner

(Stephane added explicitly since the code has his initials)

This is bit puzzling. I’ve just had a case of sound failing on a Pi that looks like it shouldn’t be possible. So far the best I can up with is a thread-safety violation.

The stacktrace shows that it was the FMSound>organ1 sound being played and that whilst handling #doControl (method that deals with the envelopes) we had a case where VolumeEnvelope>computeSlopeAtMSecs: hit a problem right at the end -
nextRecomputeTime := (mSecs + mSecsForChange) min: loopEndMSecs.
At this point the loopEndMSecs value was nil, which isn’t going to make for a happy execution of #min:.

The only method I can find that sets loopEndMSecs is Envelope>sustainEnd: and it does A Funny Thing. Initially the ivar is set to nil, then some work is done, including #computeValueAtMSecs: before the ivar is actually set to the value needed. Looking at Enveleope>computeValueAtMSecs: it seems that loopEndMSecs gets set to nil in order to fake out the testing for ‘decay phase’ at the beginning of the code.

Now it happens that #sustainEnd: is only meaningfully used by Envelope>duration: and that happens to be used by AbstractSound>stopGracefully which in turn is used by the Scratch stuff to stop a sound. So my suspicion is that we just managed to hit a tick where the SoundPlayer was triggered to Do Its Thing precisely where #sustainEnd: was between setting loopEndMSecs to nil and setting it ‘correctly’. Not likely to happen terribly often but I think we have evidence that happen it can.

I know we’ve got a lot of stuff in the image that is not thread safe but SoundPlayer is one of the cases that has a pretty good chance of being hit. I’m honestly not sure what the best way to fix this would be; probably one should argue that a sound being played and simultaneously altered requires that semaphores get involved. I can also see that probably a fairly simple change to Envelope>computeValueAtMSecs: to add a flag for ‘ignore decay phase code’ would solve this particular case.

What do you think Steph? Anyone?

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Common sense – so rare it’s a goddam superpower

timrowledge

Re: Sound playing can break in odd manner

To partly answer my own question, I just noticed that Envelope>computeValueAtMSecs: is only sent by Envelope>sustainEnd: and thus that first clause must always get skipped (because we always call it with loopEndMSecs== nil) and so surely we’d be ok to delete that bit and not mess around with the ‘pretend to be sustaining’ stuff?

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Useful random insult:- Calls people to ask them their phone number.

ccrraaiigg

re: Sound playing can break in odd manner

> To partly answer my own question, I just noticed that
> Envelope>computeValueAtMSecs: is only sent by Envelope>sustainEnd:
> and thus that first clause must always get skipped (because we always
> call it with loopEndMSecs== nil) and so surely we’d be ok to delete
> that bit and not mess around with the ‘pretend to be sustaining’
> stuff?

I agree.

-C

--
Craig Latta
netjam.org
+31 6 2757 7177 (SMS ok)
+ 1 415 287 3547 (no SMS)

Bert Freudenberg

Re: Sound playing can break in odd manner

In reply to this post by timrowledge

On 05.12.2014, at 04:11, tim Rowledge <[hidden email]> wrote:
>
> (Stephane added explicitly since the code has his initials)

We can't rely on the initials unfortunately - this is from when we converted underscore assignments to colon-equals, which originally did not preserve initials.

It would actually be a very useful project for someone to fix this: write a snippet that goes through every method, compares it to its previous version (using Chris's MC history thingy for example), and if the only change is that ':=' replaced '_' then restore the previous time stamp.

- Bert -

smime.p7s (5K) Download Attachment

J. Vuletich (mail lists)

Re: Sound playing can break in odd manner

In reply to this post by timrowledge

Hi Tim,

The Squeak process scheduler usually preempts a process only by
another one of higher priority. And when the scheduler is ready to go
back at the lower priority, it will always resume the same process
that was last suspended. The only way for two or more processes of the
same priority to share the processor is when the running process calls
#wait or #yield.

This means that using shared state from several processes at the same
priority (without using any locking mechanism, such as Semaphores,
Monitors or critical sections) is safe, as long as the processes do
#wait or #yield only when that shared state is in a consistent state.

It looks like this ARM VM is somehow not following this convention. If
this was the case, this kind of problems could arise. I just answered
with the same text to a thread in VM-Dev, where the problem shows up
when doing Morphic stuff (in Cuis) from two different processes of the
same priority. Seeing it again in a different part of a different
system, but on the same platform made me realize it must be a VM issue.

In any case, if you do your proposed change, please do it in a new
method called #computeAttackSustainValueAtMSecs: or such.
#computeValueAtMSecs: is used (at least in Cuis, jm 2/4/98) in
#showOnDisplay, that draws the envelope including the decay phase. But
if I'm right, until the assumption on the behavior of the scheduled is
'fixed', more weird shared state problems will arise.

HTH,
Juan Vuletich

Quoting tim Rowledge <[hidden email]>:

> To partly answer my own question, I just noticed that
> Envelope>computeValueAtMSecs: is only sent by Envelope>sustainEnd:
> and thus that first clause must always get skipped (because we
> always call it with loopEndMSecs== nil) and so surely we’d be ok to
> delete that bit and not mess around with the ‘pretend to be
> sustaining’ stuff?
>
> tim
> --
> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
> Useful random insult:- Calls people to ask them their phone number.

Eliot Miranda-2

Re: Sound playing can break in odd manner

Hi Juan & Tim,

On Fri, Dec 5, 2014 at 5:04 AM, J. Vuletich (mail lists) <[hidden email]> wrote:

Hi Tim,

The Squeak process scheduler usually preempts a process only by another one of higher priority. And when the scheduler is ready to go back at the lower priority, it will always resume the same process that was last suspended. The only way for two or more processes of the same priority to share the processor is when the running process calls #wait or #yield.

See http://lists.squeakfoundation.org/pipermail/vm-dev/2014-December/017215.html. Looks like the bit isn't set to ensure preemption doesn't yield. See the preemptionYields[:] accessors.

This means that using shared state from several processes at the same priority (without using any locking mechanism, such as Semaphores, Monitors or critical sections) is safe, as long as the processes do #wait or #yield only when that shared state is in a consistent state.

It looks like this ARM VM is somehow not following this convention. If this was the case, this kind of problems could arise. I just answered with the same text to a thread in VM-Dev, where the problem shows up when doing Morphic stuff (in Cuis) from two different processes of the same priority. Seeing it again in a different part of a different system, but on the same platform made me realize it must be a VM issue.

In any case, if you do your proposed change, please do it in a new method called #computeAttackSustainValueAtMSecs: or such. #computeValueAtMSecs: is used (at least in Cuis, jm 2/4/98) in #showOnDisplay, that draws the envelope including the decay phase. But if I'm right, until the assumption on the behavior of the scheduled is 'fixed', more weird shared state problems will arise.

HTH,
Juan Vuletich

Quoting tim Rowledge <[hidden email]>:

To partly answer my own question, I just noticed that Envelope>computeValueAtMSecs: is only sent by Envelope>sustainEnd: and thus that first clause must always get skipped (because we always call it with loopEndMSecs== nil) and so surely we’d be ok to delete that bit and not mess around with the ‘pretend to be sustaining’ stuff?

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Useful random insult:- Calls people to ask them their phone number.

best,

Eliot

timrowledge

Re: Sound playing can break in odd manner

I’m not entirely sure how this is related to process priority queues & handling of such. I don’t think anything about my suspicions towards the sound code would change whether pre-empted process go to the back of the queue or not; in this case the sound process pre-empts the other process, does some stuff that happens to be assuming an instvars is not nil, and if the pre-emption occurs at an inopportune moment it turns out that the instvar has been set to nil by #sustainEnd: for a short period.

So far as I can tell we’re not involving any yields to same-priority processes. It’s just some code that makes an assumption that makes an ass of it, as they so often do.

I think that either not fudging loopEndMSecs or possibly guarding the usage of it in computeSlopeAtMSecs would solve this. Or maybe a cleaner way to ‘stopGracefully’.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
People who deal with bits should expect to get bitten.

David T. Lewis

Fixing overwritten initials in the image (was: Sound playing can break in odd manner)

In reply to this post by Bert Freudenberg

On Fri, Dec 05, 2014 at 12:23:09PM +0100, Bert Freudenberg wrote:
> On 05.12.2014, at 04:11, tim Rowledge <[hidden email]> wrote:
> >
> > (Stephane added explicitly since the code has his initials)
>
> We can't rely on the initials unfortunately - this is from when we converted underscore assignments to colon-equals, which originally did not preserve initials.
>
> It would actually be a very useful project for someone to fix this: write a snippet that goes through every method, compares it to its previous version (using Chris's MC history thingy for example), and if the only change is that ':=' replaced '_' then restore the previous time stamp.
>
> - Bert -

This is a really good idea, so I am replying with a new subject line in
hopes of encouraging follow up.

Dave

Chris Muller-3

Re: Fixing overwritten initials in the image (was: Sound playing can break in odd manner)

Yes, I guess I didn't realize we had overwritten initials because I
remember doing the FixUnderscores improvement myself to preserve the
initials. Wait..

(CompiledMethod allInstances collect: [ : e | e timeStamp ] as: Bag)
sortedCounts first: 10

Dang. So, yeah, it looks like quite a few method's original
authorship information got trampled. Unfortunately, the 'mc history'
function can only show what was actually _loaded_ into that DB. IOW,
we have to have the .mcz's for the versions that were BEFORE 2/4/2006
20:41. The oldest one we have in trunk is

Name: Sound-md.6
Author: md
Time: 21 April 2006, 5:05:36 pm
UUID: 498cd464-d148-11da-a5e8-000d933a223c
Ancestors: Sound-md.5

which carries the same version of that method.

But at least we can identify the worst-offending methods (the list
above), so that maybe they could be extracted from an old image or
maybe Bob's history thing??

On Sat, Dec 6, 2014 at 2:20 PM, David T. Lewis <[hidden email]> wrote:

> On Fri, Dec 05, 2014 at 12:23:09PM +0100, Bert Freudenberg wrote:
>> On 05.12.2014, at 04:11, tim Rowledge <[hidden email]> wrote:
>> >
>> > (Stephane added explicitly since the code has his initials)
>>
>> We can't rely on the initials unfortunately - this is from when we converted underscore assignments to colon-equals, which originally did not preserve initials.
>>
>> It would actually be a very useful project for someone to fix this: write a snippet that goes through every method, compares it to its previous version (using Chris's MC history thingy for example), and if the only change is that ':=' replaced '_' then restore the previous time stamp.
>>
>> - Bert -
>
>
> This is a really good idea, so I am replying with a new subject line in
> hopes of encouraging follow up.
>
> Dave
>
>

Bert Freudenberg

Re: Fixing overwritten initials in the image (was: Sound playing can break in odd manner)

Actually we just need to take a sources/changes file from before 11/20/2005 and compare all methods to the current ones ...

- Bert -

> On 06.12.2014, at 22:20, Chris Muller <[hidden email]> wrote:
>
> Yes, I guess I didn't realize we had overwritten initials because I
> remember doing the FixUnderscores improvement myself to preserve the
> initials. Wait..
>
> (CompiledMethod allInstances collect: [ : e | e timeStamp ] as: Bag)
> sortedCounts first: 10
>
> Dang. So, yeah, it looks like quite a few method's original
> authorship information got trampled. Unfortunately, the 'mc history'
> function can only show what was actually _loaded_ into that DB. IOW,
> we have to have the .mcz's for the versions that were BEFORE 2/4/2006
> 20:41. The oldest one we have in trunk is
>
> Name: Sound-md.6
> Author: md
> Time: 21 April 2006, 5:05:36 pm
> UUID: 498cd464-d148-11da-a5e8-000d933a223c
> Ancestors: Sound-md.5
>
> which carries the same version of that method.
>
> But at least we can identify the worst-offending methods (the list
> above), so that maybe they could be extracted from an old image or
> maybe Bob's history thing??
>
>
> On Sat, Dec 6, 2014 at 2:20 PM, David T. Lewis <[hidden email]> wrote:
>> On Fri, Dec 05, 2014 at 12:23:09PM +0100, Bert Freudenberg wrote:
>>> On 05.12.2014, at 04:11, tim Rowledge <[hidden email]> wrote:
>>>>
>>>> (Stephane added explicitly since the code has his initials)
>>>
>>> We can't rely on the initials unfortunately - this is from when we converted underscore assignments to colon-equals, which originally did not preserve initials.
>>>
>>> It would actually be a very useful project for someone to fix this: write a snippet that goes through every method, compares it to its previous version (using Chris's MC history thingy for example), and if the only change is that ':=' replaced '_' then restore the previous time stamp.
>>>
>>> - Bert -
>>
>>
>> This is a really good idea, so I am replying with a new subject line in
>> hopes of encouraging follow up.
>>
>> Dave
>>
>>
>

smime.p7s (5K) Download Attachment

Chris Muller-3

Re: Fixing overwritten initials in the image (was: Sound playing can break in odd manner)

Oh yes that's the best source; the original. Maybe it can be rolled
into as part of a new .changes / .sources for the 4.6 / 5.0 release.
I liked the last thing Eliot said about it (a while back) that we
could keep all history, just get rid of the "leaves". Does anyone
remember the status of #condenseSources and #condenseChanges? Do they
just work or is there some issue which was the reason we haven't done
it in so long?

On Sat, Dec 6, 2014 at 3:32 PM, Bert Freudenberg <[hidden email]> wrote:

> Actually we just need to take a sources/changes file from before 11/20/2005 and compare all methods to the current ones ...
>
> - Bert -
>
>> On 06.12.2014, at 22:20, Chris Muller <[hidden email]> wrote:
>>
>> Yes, I guess I didn't realize we had overwritten initials because I
>> remember doing the FixUnderscores improvement myself to preserve the
>> initials. Wait..
>>
>> (CompiledMethod allInstances collect: [ : e | e timeStamp ] as: Bag)
>> sortedCounts first: 10
>>
>> Dang. So, yeah, it looks like quite a few method's original
>> authorship information got trampled. Unfortunately, the 'mc history'
>> function can only show what was actually _loaded_ into that DB. IOW,
>> we have to have the .mcz's for the versions that were BEFORE 2/4/2006
>> 20:41. The oldest one we have in trunk is
>>
>> Name: Sound-md.6
>> Author: md
>> Time: 21 April 2006, 5:05:36 pm
>> UUID: 498cd464-d148-11da-a5e8-000d933a223c
>> Ancestors: Sound-md.5
>>
>> which carries the same version of that method.
>>
>> But at least we can identify the worst-offending methods (the list
>> above), so that maybe they could be extracted from an old image or
>> maybe Bob's history thing??
>>
>>
>> On Sat, Dec 6, 2014 at 2:20 PM, David T. Lewis <[hidden email]> wrote:
>>> On Fri, Dec 05, 2014 at 12:23:09PM +0100, Bert Freudenberg wrote:
>>>> On 05.12.2014, at 04:11, tim Rowledge <[hidden email]> wrote:
>>>>>
>>>>> (Stephane added explicitly since the code has his initials)
>>>>
>>>> We can't rely on the initials unfortunately - this is from when we converted underscore assignments to colon-equals, which originally did not preserve initials.
>>>>
>>>> It would actually be a very useful project for someone to fix this: write a snippet that goes through every method, compares it to its previous version (using Chris's MC history thingy for example), and if the only change is that ':=' replaced '_' then restore the previous time stamp.
>>>>
>>>> - Bert -
>>>
>>>
>>> This is a really good idea, so I am replying with a new subject line in
>>> hopes of encouraging follow up.
>>>
>>> Dave
>>>
>>>
>>
>
>
>
>
>
>

Eliot Miranda-2

Re: Fixing overwritten initials in the image (was: Sound playing can break in odd manner)

On Sat, Dec 6, 2014 at 5:53 PM, Chris Muller <[hidden email]> wrote:

Oh yes that's the best source; the original. Maybe it can be rolled
into as part of a new .changes / .sources for the 4.6 / 5.0 release.
I liked the last thing Eliot said about it (a while back) that we
could keep all history, just get rid of the "leaves". Does anyone
remember the status of #condenseSources and #condenseChanges? Do they
just work or is there some issue which was the reason we haven't done
it in so long?

I believe they still work. I also have changes that work that implement getting rid of the leaves. I hear that there's better engineered code in Pharo too.

On Sat, Dec 6, 2014 at 3:32 PM, Bert Freudenberg <[hidden email]> wrote:
> Actually we just need to take a sources/changes file from before 11/20/2005 and compare all methods to the current ones ...
>
> - Bert -
>
>> On 06.12.2014, at 22:20, Chris Muller <[hidden email]> wrote:
>>
>> Yes, I guess I didn't realize we had overwritten initials because I
>> remember doing the FixUnderscores improvement myself to preserve the
>> initials. Wait..
>>
>> (CompiledMethod allInstances collect: [ : e | e timeStamp ] as: Bag)
>> sortedCounts first: 10
>>
>> Dang. So, yeah, it looks like quite a few method's original
>> authorship information got trampled. Unfortunately, the 'mc history'
>> function can only show what was actually _loaded_ into that DB. IOW,
>> we have to have the .mcz's for the versions that were BEFORE 2/4/2006
>> 20:41. The oldest one we have in trunk is
>>
>> Name: Sound-md.6
>> Author: md
>> Time: 21 April 2006, 5:05:36 pm
>> UUID: 498cd464-d148-11da-a5e8-000d933a223c
>> Ancestors: Sound-md.5
>>
>> which carries the same version of that method.
>>
>> But at least we can identify the worst-offending methods (the list
>> above), so that maybe they could be extracted from an old image or
>> maybe Bob's history thing??
>>
>>
>> On Sat, Dec 6, 2014 at 2:20 PM, David T. Lewis <[hidden email]> wrote:
>>> On Fri, Dec 05, 2014 at 12:23:09PM +0100, Bert Freudenberg wrote:
>>>> On 05.12.2014, at 04:11, tim Rowledge <[hidden email]> wrote:
>>>>>
>>>>> (Stephane added explicitly since the code has his initials)
>>>>
>>>> We can't rely on the initials unfortunately - this is from when we converted underscore assignments to colon-equals, which originally did not preserve initials.
>>>>
>>>> It would actually be a very useful project for someone to fix this: write a snippet that goes through every method, compares it to its previous version (using Chris's MC history thingy for example), and if the only change is that ':=' replaced '_' then restore the previous time stamp.
>>>>
>>>> - Bert -
>>>
>>>
>>> This is a really good idea, so I am replying with a new subject line in
>>> hopes of encouraging follow up.
>>>
>>> Dave
>>>
>>>
>>
>
>
>
>
>
>

best,

Eliot

Bert Freudenberg

Re: Fixing overwritten initials in the image (was: Sound playing can break in odd manner)

In reply to this post by Chris Muller-3

On 06.12.2014, at 22:20, Chris Muller <[hidden email]> wrote:
>
> Yes, I guess I didn't realize we had overwritten initials because I
> remember doing the FixUnderscores improvement myself to preserve the
> initials. Wait..
>
> (CompiledMethod allInstances collect: [ : e | e timeStamp ] as: Bag)
> sortedCounts first: 100

This statistic is really useful!

I fixed some of these using the attached method. Pushed the result to trunk.

The statistic looks much better now, but there's still some curious clusters. Maybe someone else could investigate those ...

- Bert -

ChangeSet class-restoreTimeStampsFrom.st (2K) Download Attachment

smime.p7s (5K) Download Attachment