[Csantucci]

Dennis,
as no one else was tackling the problem, I have tried applying your patch, as I was continuing to get crashes. Thank you very much for your proposal! What I found out is that removing the distinction between standard and frequent update sounds, as you did, significantly increases CPU load for the sound process and decreases FPS. So I tried re-introducing the distinction. Even so, CPU load is still higher than before your patch, however effect on FPS seems smaller, at least in my test case. Before committing anything, I attach here the patch I am using, and would like you to have a look and to give an opinion, as not everything you have implemented is clear to me, so maybe there is a fault in my changes; if there is a suggestion to further reduce CPU load towards the original value, it is welcome.

 SoundProcess_mio.cs.patch.zip (2.24K)
Number of downloads: 204

[dennisat]

Csantucci, on 02 January 2015 - 08:26 AM, said:

I attach here the patch I am using, and would like you to have a look and to give an opinion, as not everything you have implemented is clear to me,

I'll look at it as soon as possible. I've refined my original patch somewhat because it was a first attempt and I discovered some poor code. It damaged nothing and did stop the crashes but some of the things I thought I was doing didn't work! That may be why your resource usage increased. Also, the statistics are not necessary to the function; I was just trying to get an idea of how long it took to process the sound sources in some heavy activities.

Dennis

[dennisat]

dennisat, on 02 January 2015 - 11:11 AM, said:

I'll look at it as soon as possible.

You have got the essence of my idea Carlo, but my restart after interrupt logic was wrong. In fact, it just started from the beginning of the update again instead of approximately where it had been interrupted. I was generating increased resource usage although the time code was locked was decreased and the crashes avoided. I am being called to dinner right now, I'll produce a modification of your patch for you to review tomorrow, I hope.

Dennis

[dennisat]

dennisat, on 02 January 2015 - 12:25 PM, said:

.. I'll produce a modification of your patch for you to review tomorrow, I hope..

Hi Carlo,

I've adjusted your patch to correct my error with the restart and also altered a few comments. I notice you've put all the statistics coding into debug blocks. Because of some poor coding on my part, the statistics produced were not reliable! I suggest you get rid of them or rework them to produce something meaningful if you're going to use this patch.

Using lock was the original method of protection of the data and it was replaced by Interlocked Compare/Exchange which unfortunately does not seem to protect the Lists that the Dictionary points to.

To get rid of the random errors I therefore reverted to using lock. This has major disadvantages, especially with the sound processing, because I've measured some prolonged periods on sound dense activities (more than 5 minutes) when it was taking more than 300ms to process the sound updates. During this 300ms, which is an eon in modern computing, adds and deletes to the sound sources could not be processed. Since loading tiles can cause a lot of sound source additions and deletions, I thought this could be a contributory cause of sound stuttering. I therefore decided to try allowing the sound source update to be interrupted when additions and deletions were required. Subjectively, stuttering seems to have decreased but I've got no hard data to support this - how do you measure degree of stutter?

I've used Interlocked Compare/Exchange to manage an interrupt switch which, in my old IBM mainframe days, used to be called a "spin lock". You can probably see why. Event signalling may be a better way of doing this in C# but my understanding of C# hasn't got that far yet.

Dennis

 SoundProcess_mio_1.cs.patch.zip (2.44K)
Number of downloads: 204

[Csantucci]

Dennis,
thank you for your new patch. I tested it, and at a first test I got more or less the same performances as with the previous patch version, that is sound process requires about 20-25% more CPU load than without patch (that is if e.g. it was 40% before patch, it is 50% now); however this seems not to have bad effects on the FPS; on the contrary, in a specific medium-load case, FPS seem higher with patch than without patch (and also higher than with the previous version of the patch)! Before committing I would like to see if there is some pertinent comment on this.

[dennisat]

Csantucci, on 03 January 2015 - 08:21 AM, said:

.. sound process requires about 20-25% more CPU load than without patch...

I think you can bring that down by restoring sleeptime to a fixed 50ms. This may be better for processors with less than 4 cores. A variable sleeptime can take better advantage of a 4 (or more) core processor because it will not matter if the sound thread runs up to 100%. Other threads will hardly be affected and may even benefit by getting sound processes on their behalf through quicker. On a 2 core though, the sound process will be sharing with other processes and they may be degraded if it is allowed too much resource.

Dennis

[Csantucci]

I've uploaded your patch in release 2755 restoring the fixed 50 ms. I've only commented the lines that provide variable sleep time, so that in a future time they could be considered again for insertion.
Dennis, thank you again for your fix.

[James Ross]

I can't believe you've reintroduced that terrible lock in this code. :good:

Expect me to remove it again in the future.

[dennisat]

James Ross, on 03 January 2015 - 12:17 PM, said:

I can't believe you've reintroduced that terrible lock in this code. :good:

Expect me to remove it again in the future.

You might well find a better way. However, I run sound laden routes and activities (like MEP and Dorset Coast) and the crashes were incessant - it was as bad as MSTS. I did spend quite a while trying to get the Interlocked Compare/Exchange method to work but whatever variety I tried, it did not seem to protect (ie copy) the lists that the dictionary pointed to. I was starting to think that you'd have to individually copy these lists but that would be barmy because you'd probably have to use lock to copy them all in sync! In the end I gave up and took the easy way out. The update, the major bottleneck, is now interruptible so I don't think there'll be much advantage in getting the Interlocked Compare/Exchange method to work.

I suspect you'll take that as a challenge! I certainly would.

Best of luck
Dennis

[Csantucci]

As always, better solutions are welcome. However, after about two months of crashes, this at least avoids the crashes.

[James Ross]

It's weird, as I've never had a single crash from this code. But I am sure that we can do better than a lock. ;)

For what it's worth, I don't believe the following to be true:

dennisat, on 06 December 2014 - 01:23 PM, said:

I understand that compare/exchange can only protect contiguous items of data, not many separate items which is what a dictionary must usually be.

Compare/exchange simply allows you to update a pointer atomically - it doesn't matter what kind of data it is. It does, however, matter that the data structures using it are immutable, which is why the code was quite complex: to add or remove a sound source (in a List in the Dictionary) it must create a new List (with the item added/removed) and a new Dictionary, then swap the new and old Dictionaries. Judging by the crash stack, something, somewhere is not correctly doing this and is instead modifying a List that is currently in-use. (Unfortunately there's no good way to validate that data structures are immutable in .NET.)

It would be really useful if someone could provide some example situations when this occurs which I might be able to test with since, like I said, I don't believe I've seen this happen even once. (Log files are useful here in case you have different settings than me, for example.)

[dennisat]

James Ross, on 03 January 2015 - 02:31 PM, said:

For what it's worth, I don't believe the following to be true:
I understand that compare/exchange can only protect contiguous items of data, not many separate items which is what a dictionary must usually be...

It must be true, I found it on the Internet! ;) One of the many things I read while trying to find a way to use compare/exchange on collections of collections. Being an ex-assembler programmer it did make a kind of sense.

However, I also know it should be possible to design a method which would cater for non-contiguous collections of collections. I just never got any of the options to work and took the easy way out. However, as I said above, to mitigate the effects of using lock, I've made the sound update interruptible.

As for a reliable test case, it doesn't seem to be possible to get a regular failure at one point. Probably the variables are too many. Something in the background might kick in, slightly altering timings in the program and hey presto! - No crash or crash at a different point. A crash can usually be created by starting an intense activity on MEP or Dorset Coast that runs through the most complex areas. It may happen in minutes or take an hour but it will crash. Unfortunately it never happens at the same place twice.

Dennis

[Csantucci]

James Ross, on 03 January 2015 - 02:31 PM, said:

It's weird, as I've never had a single crash from this code. ...
It would be really useful if someone could provide some example situations when this occurs which I might be able to test with since, like I said, I don't believe I've seen this happen even once. (Log files are useful here in case you have different settings than me, for example.)

As dennisat says, it's random. The only thing I can say is that to me usually the crash occurs when an AI train is created near to the player train or arrives at the distance to the player train where its sound sources enter into the dictionary.

[James Ross]

Csantucci, on 04 January 2015 - 12:19 AM, said:

As dennisat says, it's random. The only thing I can say is that to me usually the crash occurs when an AI train is created near to the player train or arrives at the distance to the player train where its sound sources enter into the dictionary.

Sure, but random things are more or less likely based on a number of environmental factors. Since I have never seen this happen I must be in some kind of favourable environment, where as you are not. The game settings are part of that, as is the route/activity. I'll try with MEP but without knowing more about your environment I may still not be able to reproduce the problem.

[dennisat]

James Ross, on 04 January 2015 - 03:54 AM, said:

Sure, but random things are more or less likely based on a number of environmental factors...

A thought. Changing sleep time in soundprocess.cs from 50ms to something lower, such as 5ms, would get the update turning over much faster and may increase the chances of a clash with add and delete sound sources.

Dennis