PDA

View Full Version : Scrolling - prototype high priority timers



Triode
2005-03-12, 13:58
Dan, Vidur, kdf

Please see attached prototype of an addition high priority timer queue. This provides a second queue of timers which animation
functions can use (using setHighTimer rather than setTimer). Also included how I am using it for SqueezeboxG.pm

New logic:
- Whenever checkTimers is called, the high priority queue is checked first
- Either all high priority timers which are due are processed and checkTimers returns, or one normal timer is processed
- If already within a normal timer only high priority ones can execute (two steps to the previously implemented semaphore)
- If already executing a high priority timer then don't execute a new high priority timer.

All other functions extended to deal with the two queues (perhaps some of this could be done more elegantly)

The only function I am uncertain about is firePendingTimer - this currently doesn't obey the rules about not firing timers within
timers - its only used in IR.pm at present. What do you think the correct rules for this should be?

Anyway, what do you think? Thus far I have not found any side effects and it sees to reduce jitter of scrolling during web page
updates, calls to HTTP->content etc

Adrian

[BTW I notice there are a few redundant global variables in Timers.pm?]

Dan Sully
2005-03-12, 14:05
* Triode shaped the electrons to say...

>New logic:
>- Whenever checkTimers is called, the high priority queue is checked first
>- Either all high priority timers which are due are processed and
>checkTimers returns, or one normal timer is processed
>- If already within a normal timer only high priority ones can execute (two
>steps to the previously implemented semaphore)
>- If already executing a high priority timer then don't execute a new high
>priority timer.
>
>All other functions extended to deal with the two queues (perhaps some of
>this could be done more elegantly)

Instead of:

foreach my $timer (@highTimers) {
if (($timer->{'client'} eq $client) && ($timer->{'subptr'} eq $subptr) ) {
$count++;
}
}

foreach my $timer (@normalTimers) {
if (($timer->{'client'} eq $client) && ($timer->{'subptr'} eq $subptr) ) {
$count++;
}
}

You can do:

foreach my $timer (@highTimers, @normalTimers) {

if (($timer->{'client'} eq $client) && ($timer->{'subptr'} eq $subptr) ) {
$count++;
}
}

>The only function I am uncertain about is firePendingTimer - this currently
>doesn't obey the rules about not firing timers within timers - its only
>used in IR.pm at present. What do you think the correct rules for this
>should be?

Vidur or Dean will need to get this one.

>Anyway, what do you think? Thus far I have not found any side effects and
>it sees to reduce jitter of scrolling during web page updates, calls to
>HTTP->content etc

Looks good to me so far. Have you done any testing on non-G SB or SliMP3?

>[BTW I notice there are a few redundant global variables in Timers.pm?]

Those can probably go away.

-D
--
Ya gotta love UNIX, where else do you wonder whether
you can kill a zombie spawned by a daemon's fork?

kdf
2005-03-12, 14:10
Quoting Dan Sully <dan (AT) slimdevices (DOT) com>:

> foreach my $timer (@highTimers, @normalTimers) {

btw, this reminds me of something. I've been seeing a lot of changing of
foreach my ...
to
for my ...

is there a hard and fast style ruling on this?

-kdf

Triode
2005-03-12, 14:12
> You can do:
>
> foreach my $timer (@highTimers, @normalTimers) {
>
> if (($timer->{'client'} eq $client) && ($timer->{'subptr'} eq $subptr) ) {
> $count++;
> }
> }

Thanks - I knew there must be a better way!

>
> Looks good to me so far. Have you done any testing on non-G SB or SliMP3?
>

No afraid not, I only SBG and Softsqueeze (don't have the hardware).

Adrian

Dan Sully
2005-03-12, 14:13
* kdf shaped the electrons to say...

>btw, this reminds me of something. I've been seeing a lot of changing of
>foreach my ...
>to
>for my ...
>
>is there a hard and fast style ruling on this?

No - just my neurosis. They are exactly the same thing.

-D
--
Ya gotta love UNIX, where else do you wonder whether
you can kill a zombie spawned by a daemon's fork?

kdf
2005-03-12, 14:28
Quoting Dan Sully <dan (AT) slimdevices (DOT) com>:

> * kdf shaped the electrons to say...
>
> >btw, this reminds me of something. I've been seeing a lot of changing of
> >foreach my ...
> >to
> >for my ...
> >
> >is there a hard and fast style ruling on this?
>
> No - just my neurosis. They are exactly the same thing.
>
That's what I figured. Functionally the same, but it is a style thing. If we
want to always leave out the 'each' I'll keep it out to start with, instead of
sending you into nervous twitches until you change it yourself ;)

-kdf

kdf
2005-03-12, 21:57
Quoting Triode <triode1 (AT) btinternet (DOT) com>:

> Dan, Vidur, kdf
>
> Please see attached prototype of an addition high priority timer queue. This
> provides a second queue of timers which animation
> functions can use (using setHighTimer rather than setTimer). Also included
> how I am using it for SqueezeboxG.pm
>
ok, based on comparison with previous performance....wow

there is still a bit of jitter, just at the initiation of an http request. Ull
refresh of the browser is worse than, say, clicking a browse mode. Bring on
Dave's non-blocking code and I expect this to be nice.

I'm at only 4.7& cpu, with 4 players doing rss feeds and one doing a live365
stream with vis.

the question is, what is going to happen if plugins start using hightimers for
the 'wrong' things? is it possible this could end up overloaded?
-kdf

Triode
2005-03-13, 03:48
Glad you like the overall result..

> there is still a bit of jitter, just at the initiation of an http request. Ull
> refresh of the browser is worse than, say, clicking a browse mode. Bring on
> Dave's non-blocking code and I expect this to be nice.
>
> I'm at only 4.7& cpu, with 4 players doing rss feeds and one doing a live365
> stream with vis.

You probably still get some jitter from the the live365 stream too as looking for metadata is still blocking at present.

>
> the question is, what is going to happen if plugins start using hightimers for
> the 'wrong' things? is it possible this could end up overloaded?

I think the worst case is that it tends towards previous performance with plugins setting very stort duration timers. Its always
possible to steal the timer queue by placing yourself at the start and keep putting yourself back there! It would cause loss of
real timeness though if whatever was placed on the high timer queue took a long time to execute. I can change the --d_perf check to
a very low value to attempt to find this (0.5 seconds is far too long for a timer to run anyway..)

I will have a look a this and proposals for where to use high timers in the Slimp3/SB1 animations [now I've looked at that code...]

Vidur/Dean - any comments on the firePendingTimer case (see first post in thread)?

Adrian

Triode
2005-03-13, 06:19
Slim::Web::Pages::buildPlaylist appears to call idleStream once per loop, but not for the first 0.25 seconds (hence starving
streaming and animation for the first 0.25 of a second)

Could we make it always call idleStreams once a per loop? Alternatively change so that it calls it every X ms [but I don't think
idleStreams is too expensive if there is nothing to select or no timers pending]

Adrian
----- Original Message -----
From: "kdf" <slim-mail (AT) deane-freeman (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Sunday, March 13, 2005 4:57 AM
Subject: Re: [Developers] Scrolling - prototype high priority timers


> Quoting Triode <triode1 (AT) btinternet (DOT) com>:
>
>> Dan, Vidur, kdf
>>
>> Please see attached prototype of an addition high priority timer queue. This
>> provides a second queue of timers which animation
>> functions can use (using setHighTimer rather than setTimer). Also included
>> how I am using it for SqueezeboxG.pm
>>
> ok, based on comparison with previous performance....wow
>
> there is still a bit of jitter, just at the initiation of an http request. Ull
> refresh of the browser is worse than, say, clicking a browse mode. Bring on
> Dave's non-blocking code and I expect this to be nice.
>
> I'm at only 4.7& cpu, with 4 players doing rss feeds and one doing a live365
> stream with vis.
>
> the question is, what is going to happen if plugins start using hightimers for
> the 'wrong' things? is it possible this could end up overloaded?
> -kdf
>

Dan Sully
2005-03-13, 10:44
* Triode shaped the electrons to say...

>Slim::Web::Pages::buildPlaylist appears to call idleStream once per loop,
>but not for the first 0.25 seconds (hence starving streaming and animation
>for the first 0.25 of a second)
>
>Could we make it always call idleStreams once a per loop? Alternatively
>change so that it calls it every X ms [but I don't think idleStreams is too
>expensive if there is nothing to select or no timers pending]

Yes - please make this better. Currently playlist building / displaying sucks.

Post 6.0, playlists need to be generated in the DB - possibly using the
existing playlist_track table, with a magic url in the tracks table of
client://00:04:20:XX:XX:XX

Walking the filesystem to generate these pages, which refresh often, and can
be quite large is really the wrong way to do this.

Any patches to help would be greatly appreciated.

-D
--
They're techno trousers, ex-NASA, fantastic for walkies!

Triode
2005-03-13, 12:46
>>Slim::Web::Pages::buildPlaylist appears to call idleStream once per loop, but not for the first 0.25 seconds (hence starving
>>streaming and animation for the first 0.25 of a second)
>>
>>Could we make it always call idleStreams once a per loop? Alternatively change so that it calls it every X ms [but I don't think
>>idleStreams is too expensive if there is nothing to select or no timers pending]
>
> Yes - please make this better. Currently playlist building / displaying sucks.
>

I think I've stumbled over a reentrancy issue for select here. If I put more idleStreams() calls into the HTTP code (or just call
it more often), I increase the chance of the http server code blocking the whole server for 10 seconds. I think this is what's
happening:

1) The HTTP code (Slim::Web::HTTP) changes the timeout for any new client socket to 10 seconds (Keepalive interval) once the socket
is established

2) Slim::Web::HTTP::processHTTP is added to to the select read list when a new HTTP session is established.

3) Within processHTTP, its possible to execute idleStreams() [and I would like it to do this more!]

4) Slim::Networking::Select::select is'nt really reentrant? It attempts to process all file handles which return from select.
However if it executes a callback such as processHTTP, this may execute idleStreams in the middle of it...

So its seems to be possible for select to call a callback on a handle which it has already been serviced and hence even though
select calls the callback it may not be possible to read or write to the handle [Because idleStreams was run inside a previous
function called from select]

To avoid this I think there are couple of options:

1) Slim::Network::select should return as soon as it has executed a single callback - i.e. not attempt to execute calbacks on all
handles (possibly small performance penalty)?

2) Ensure all callbacks called from select are able to cope with nothing to read or write and don't block the server if this is the
case
In the case of the http code this means not setting the timeout value for the http socket should to 10 seconds.

Is there any serious problem with avoiding the following line in Slim::Web::HTTP (~line 271)
$httpClient->timeout(KEEPALIVETIMEOUT)

Any views?

Adrian

Dan Sully
2005-03-13, 12:55
* Triode shaped the electrons to say...

>I think I've stumbled over a reentrancy issue for select here. If I put
>more idleStreams() calls into the HTTP code (or just call
>it more often), I increase the chance of the http server code blocking the
>whole server for 10 seconds. I think this is what's
>happening:
>
>1) The HTTP code (Slim::Web::HTTP) changes the timeout for any new client
>socket to 10 seconds (Keepalive interval) once the socket
>is established
>
>2) Slim::Web::HTTP::processHTTP is added to to the select read list when a
>new HTTP session is established.
>
>3) Within processHTTP, its possible to execute idleStreams() [and I would
>like it to do this more!]
>
>4) Slim::Networking::Select::select is'nt really reentrant? It attempts to
>process all file handles which return from select.
>However if it executes a callback such as processHTTP, this may execute
>idleStreams in the middle of it...
>
>So its seems to be possible for select to call a callback on a handle which
>it has already been serviced and hence even though select calls the
>callback it may not be possible to read or write to the handle [Because
>idleStreams was run inside a previous function called from select]
>
>To avoid this I think there are couple of options:
>
>1) Slim::Network::select should return as soon as it has executed a single
>callback - i.e. not attempt to execute calbacks on all handles (possibly
>small performance penalty)?

Have you done any testing on this? I'm worried that it may starve streams that way.

>2) Ensure all callbacks called from select are able to cope with nothing to
>read or write and don't block the server if this is the case
>In the case of the http code this means not setting the timeout value for
>the http socket should to 10 seconds.
>
>Is there any serious problem with avoiding the following line in
>Slim::Web::HTTP (~line 271)
>$httpClient->timeout(KEEPALIVETIMEOUT)

This would seem like the better (and possibly easier?) choice. However, I
don't know what effect it would have on the HTTP keepAlive connections. I
wrote that code, but it seems to have fallen out of my brain right now.

-D
--
<faisal> my life is collapsing to what will soon be NEGATIVE INTEGER degrees of separation.

Triode
2005-03-13, 13:22
>>1) Slim::Network::select should return as soon as it has executed a single callback - i.e. not attempt to execute calbacks on all
>>handles (possibly small performance penalty)?
>
> Have you done any testing on this? I'm worried that it may starve streams that way.
>

I've tested for functionality, but not performance with lots of streaming sessions - yes it is likely to cause the idle loop to
execute more often.. However it does fix this problem indicating that select is currently calling some callbacks too often.

I will think about whether we can put some flags in select to make it reentrant - its only the case where a callback executes select
and this then invalidates the previous list of callbacks to call in some way.

How does perl handle the foreach:

foreach my $sock (@$r) {
my $readsub = $readCallbacks{"$sock"};
$readsub->($sock) if $readsub;
}

If $sock and @r are changed mid way though (as $readsub calls select again)?

>>2) Ensure all callbacks called from select are able to cope with nothing to read or write and don't block the server if this is
>>the case
>>In the case of the http code this means not setting the timeout value for the http socket should to 10 seconds.
>>
>>Is there any serious problem with avoiding the following line in Slim::Web::HTTP (~line 271)
>>$httpClient->timeout(KEEPALIVETIMEOUT)
>
> This would seem like the better (and possibly easier?) choice. However, I
> don't know what effect it would have on the HTTP keepAlive connections. I
> wrote that code, but it seems to have fallen out of my brain right now.
>

Yes I prefer this - perhaps you could have a think about the implications of this.

Adrian

Dan Sully
2005-03-13, 13:25
* Triode shaped the electrons to say...

>I will think about whether we can put some flags in select to make it
>reentrant - its only the case where a callback executes select and this
>then invalidates the previous list of callbacks to call in some way.
>
>How does perl handle the foreach:
>
>foreach my $sock (@$r) {
> my $readsub = $readCallbacks{"$sock"};
> $readsub->($sock) if $readsub;
>}
>
>If $sock and @r are changed mid way though (as $readsub calls select again)?

It will ignore any changes to $r, but $sock changes will probably be ok.

>>This would seem like the better (and possibly easier?) choice. However, I
>>don't know what effect it would have on the HTTP keepAlive connections. I
>>wrote that code, but it seems to have fallen out of my brain right now.
>
>Yes I prefer this - perhaps you could have a think about the implications of this.

Sure. What's the test case? Just commenting it out?

I'm also seeing some wierd 100% cpu usage, with either normal or internet radio playing.

Trying to track it down - was wondering if anyone else is seeing it?

-D
--
There is no emergency. Nothing to see here. Move along.

Triode
2005-03-13, 13:40
>>foreach my $sock (@$r) {
>> my $readsub = $readCallbacks{"$sock"};
>> $readsub->($sock) if $readsub;
>>}
>>
>>If $sock and @r are changed mid way though (as $readsub calls select again)?
>
> It will ignore any changes to $r, but $sock changes will probably be ok.
>

So if $r = 1, 2, 3, 4
callback for 1 OK
calback for 2 causes excution of idleSelect:
new $r = 3, 4
execute callback for 3
execute callback for 4
return
$r now = 1, 2, 3, 4 ?
$sock = 2 or 4 and does it loop again?

>>Yes I prefer this - perhaps you could have a think about the implications of this.
>
> Sure. What's the test case? Just commenting it out?

Yes - just commented the line out!

>
> I'm also seeing some wierd 100% cpu usage, with either normal or internet radio playing.
>

Is this in general or related to the web server (saw a post on this) - not seen it myself.

Adrian

Dan Sully
2005-03-13, 13:45
* Triode shaped the electrons to say...

>So if $r = 1, 2, 3, 4
>callback for 1 OK
>calback for 2 causes excution of idleSelect:
> new $r = 3, 4
> execute callback for 3
> execute callback for 4
>return
>$r now = 1, 2, 3, 4 ?

Yes.

>$sock = 2 or 4 and does it loop again?

$sock is 2

>>Sure. What's the test case? Just commenting it out?
>
>Yes - just commented the line out!

Will try.

>>I'm also seeing some wierd 100% cpu usage, with either normal or internet radio playing.
>
>Is this in general or related to the web server (saw a post on this) - not seen it myself.

In general, which is what worries me.

-D
--
<dr.pox> do they call it 'gq' because it makes your text fashionable?

Triode
2005-03-13, 14:07
>>>I'm also seeing some wierd 100% cpu usage, with either normal or internet radio playing.
>>
>>Is this in general or related to the web server (saw a post on this) - not seen it myself.
>
> In general, which is what worries me.
>

Hum. If its my fault, then try the following in Timers.pm:

sub nextTimer {
return (undef) if (!scalar(@timers) || checkingTimers ); <<<< additional clause here!

Adrian

Dan Sully
2005-03-13, 14:23
* Triode shaped the electrons to say...

>>In general, which is what worries me.
>>
>
>Hum. If its my fault, then try the following in Timers.pm:
>
>sub nextTimer {
>return (undef) if (!scalar(@timers) || checkingTimers ); <<<< additional
>clause here!

That looks to have done it. Missing $ though.

-D
--
"My pockets hurt." - Homer Simpson

Triode
2005-03-13, 14:30
>>return (undef) if (!scalar(@timers) || checkingTimers ); <<<< additional clause here!
>
> That looks to have done it. Missing $ though.
>

I think this only catches the case where a timer is set in the future and HTTP->content() is called from within a timer. I was
calling idleStreams() with a timeout of 0.1 in this case, but it would be reset to 0 by the next timer test and then the timer is
blocked from executing.

So hopefully this was the problem.

Appologies for not thinking of this before - it came to me after you reported it.

Could you check this in as my timer code is a bit all over the place at present..

Adrian

Dan Sully
2005-03-13, 18:02
* Triode shaped the electrons to say...

>>>Is there any serious problem with avoiding the following line in
>>>Slim::Web::HTTP (~line 271)
>>>$httpClient->timeout(KEEPALIVETIMEOUT)
>>
>>This would seem like the better (and possibly easier?) choice. However, I
>>don't know what effect it would have on the HTTP keepAlive connections. I
>>wrote that code, but it seems to have fallen out of my brain right now.
>>
>
>Yes I prefer this - perhaps you could have a think about the implications of this.

So without a timeout - the client connection could hang forever

-D
--
<dr.pie> 31336.5: the neighbor of the l33t

Triode
2005-03-14, 11:32
>>>>Is there any serious problem with avoiding the following line in Slim::Web::HTTP (~line 271)
>>>>$httpClient->timeout(KEEPALIVETIMEOUT)
>
> So without a timeout - the client connection could hang forever
>

OK - that counts that out!

So at present there is a small, but finite chance of the server blocking for 10 seconds if select calls the http code twice for the
same input [very reproducable with more use of idleStreams() inside the http code and hammering the web page with refreshes.., but
does not seem to happen in the normal case]

I think this can be resolved by changing the select processing, but clearly this would be a critical change so will think about it a
bit and we need to discuss more widely...

Adrian

Dan Sully
2005-03-14, 11:37
* Triode shaped the electrons to say...

>>>>>Is there any serious problem with avoiding the following line in
>>>>>Slim::Web::HTTP (~line 271)
>>>>>$httpClient->timeout(KEEPALIVETIMEOUT)
>>
>>So without a timeout - the client connection could hang forever
>
>So at present there is a small, but finite chance of the server blocking
>for 10 seconds if select calls the http code twice for the same input [very
>reproducable with more use of idleStreams() inside the http code and
>hammering the web page with refreshes.., but does not seem to happen in the
>normal case]
>
>I think this can be resolved by changing the select processing, but clearly
>this would be a critical change so will think about it a bit and we need to
>discuss more widely...

Agreed. Could you give a quick summary of where the current timer / select
code stands, and what it's issues are? And what code (if any) you have waiting?

Thanks.

-D
--
"They that can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety." - Benjamin Franklin

Triode
2005-03-14, 11:59
> Agreed. Could you give a quick summary of where the current timer / select
> code stands, and what it's issues are? And what code (if any) you have waiting?
>

OK.

1) I believe the current timer code is safe (with my fix from yesterday). This does block timers whenever an existing timer
callback is already being run, so actually blocks annimations during any thing that is initiated by a timer. This could stand on
its own if you wanted.

2) I have a working version of my dual timer code - adding an additional high priority timer for annimations. I need to review this
again tonight (now) as I was trying to optimise it slightly yesterday. This gets annimations whilst doing web updates etc. I will
post later for code review and hopefully a few to test - pending testing I would hope this could be considered for inclusion.

3) The select/http problem I believe is OK to leave for the moment - I think it could occur in current code and is not really
changed by the timer changes. Its chance of occuring is
only if there are http requests and something calling idleStreams(). What needs to happen is select returning multiple read
handles, with the http request socket later in the list - if one of the callbacks earlier in the list calls idleStreams() it may
service the http request before the original select routine gets round to it. I could only reproduce it by hammering the sever with
http requests, but it is possible. The new non blocking http code in 6.0 could argueably increase the probability slightly - but
this was not my test condition. Conclusion - safe for now, but ideally improve later - worst case is whole server blocks for 10
seconds.

Adrian

Triode
2005-03-14, 12:24
> 1) I believe the current timer code is safe (with my fix from yesterday). This does block timers whenever an existing timer
> callback is already being run, so actually blocks annimations during any thing that is initiated by a timer. This could stand on
> its own if you wanted.
>
> 2) I have a working version of my dual timer code - adding an additional high priority timer for annimations. I need to review
> this again tonight (now) as I was trying to optimise it slightly yesterday. This gets annimations whilst doing web updates etc.
> I will post later for code review and hopefully a few to test - pending testing I would hope this could be considered for
> inclusion.
>
> 3) The select/http problem I believe is OK to leave for the moment - I think it could occur in current code and is not really
> changed by the timer changes. Its chance of occuring is
> only if there are http requests and something calling idleStreams(). What needs to happen is select returning multiple read
> handles, with the http request socket later in the list - if one of the callbacks earlier in the list calls idleStreams() it may
> service the http request before the original select routine gets round to it. I could only reproduce it by hammering the sever
> with http requests, but it is possible. The new non blocking http code in 6.0 could argueably increase the probability slightly -
> but this was not my test condition. Conclusion - safe for now, but ideally improve later - worst case is whole server blocks for
> 10 seconds.
>
Got the last bit wrong re the 6.0 http code. HTTP->content() is not added to select so this does not cause the problem. It's only
the http web server code and hence is as likely as it has always been - so OK to leave for now.

I can look at a new select routine later this week/this weekend if appropriate.

Adrian

Nalle Johansson
2005-03-19, 02:31
A have noticed that since I upgraded to 6.x I cant play
songs that have a swedish character in its name.

"" - And the box is displaying a weard character
instead.


Bsta hlsningar/Best Regards
Nalle Johansson

Dan Sully
2005-03-19, 02:36
* Nalle Johansson shaped the electrons to say...

>A have noticed that since I upgraded to 6.x I cant play
>songs that have a swedish character in its name.
>
>"åäöÅÄÖ" - And the box is displaying a weard character instead.

Nalle - some more information about your system please? Perl 5.6 or 5.8?

The types of files these are?

Your locale, if you know it.

Thanks.

-D
--
"It has become appallingly obvious that our technology has exceeded our humanity." - Albert Einstein

Nalle Johansson
2005-03-19, 03:02
Slim Devices Developers <developers (AT) lists (DOT) slimdevices.com>
on den 19 mars 2005 at 10:36 +0100 wrote:
>* Nalle Johansson shaped the electrons to say...
>
>>A have noticed that since I upgraded to 6.x I cant play
>>songs that have a swedish character in its name.
>>"" - And the box is displaying a weard character
>instead.
>Nalle - some more information about your system please?
>Perl 5.6 or 5.8?
>The types of files these are?
>Your locale, if you know it.

I run the windows version on a windows 2KServer and 11
slimdevices
all my files are .mp3
Just tryed the latest download and the same problem there.
If I rename the file from "ke.mp3" to "ake.mp3" the player
will play the song. Now it says somethone like "????ke.mp3
can not be found"


Bsta hlsningar/Best Regards
Nalle Johansson