Scott Hanselman

Back To Basics - Everyone Remember Where We Parked (that memory)!

July 11, 2008 Comment on this post [37] Posted in BabySmash | Back to Basics
Sponsored By

I added a new feature to BabySmash during lunch, so that if your (baby's) mouse wheel is over a shape and they scroll the wheel, the system will play a sound and zoom that object in or out. The mouse wheel events come REALLY fast, as do most mouse events.

The general idea is this. I've got the PInvoke/DllImport call to the PlaySound API and a couple of helper methods. If the WAV wasn't cached, I'd go get it and store it away. All this code was kind of written on auto-pilot, you know? It's called very quickly in the MouseWheel event and works fine...until it totally doesn't work at all.

I found that when I wheeled the mouse REALLY fast, sometimes it'd get a nasty burst of loud static instead of the nice WAV file playing as I expected.

I store my WAV files inside the resources of BabySmash.exe (for now) so I just hold them in memory. Initially I would pull them out of the resource every time, but then I added some basic caching. (I probably should have used Chad and Jeremy's cache, but anyway)

[DllImport("winmm.dll")]
public static extern bool PlaySound(byte[] data, IntPtr hMod, UInt32 dwFlags);

public static void PlayWavResource(string wav)
{
byte[] b = GetWavResource(wav);
PlaySound(b, IntPtr.Zero, SND_ASYNC | SND_MEMORY);
}

public static void PlayWavResourceYield(string wav)
{
byte[] b = GetWavResource(wav);
PlaySound(b, IntPtr.Zero, SND_ASYNC | SND_MEMORY | SND_NOSTOP);
}

private static byte[] GetWavResource(string wav)
{
//TODO: Is this valid double-check caching?
byte[] b = null;
if (cachedWavs.ContainsKey(wav))
b = cachedWavs[wav];
if (b == null)
{
lock (cachedWavsLock)
{
// get the namespace
string strNameSpace = Assembly.GetExecutingAssembly().GetName().Name;

// get the resource into a stream
using (Stream str = Assembly.GetExecutingAssembly().GetManifestResourceStream(strNameSpace + wav))
{
if (str == null)
throw new ArgumentException(wav + " not found!");
// bring stream into a byte array
var bStr = new Byte[str.Length];
str.Read(bStr, 0, (int)str.Length);
cachedWavs.Add(wav, bStr);
return bStr;
}
}
}
return b;
}

Anyway, I kind of forgot that byte was a value type and in a chat this afternoon Larry made this comment. You might remember that the man responsible for the PlaySound() API is none other than Larry Osterman, who I interviewed last year. Here's our chat transcript:

Larry Osterman‎‎:
My guess is that you're deleting the array b before the PlaySound has completed.
or rather the CLR is.

‎‎Scott Hanselman:
even though it's on the stack?
ah
I get it
the GC is getting to it

‎‎Larry Osterman‎‎:
when you say snd_async, it queues the actual playsound operation to a worker thread.
Yup, GC makes it go away.

When I started going really fast with dozens of calls to PlaySound() a second, I was piling these up and eventually hit the point where one byte[] that was being played would disappear (get garbage collected) and I'd hear the sound of zeros being played. Which sounds much like static. (kidding) ;) I could have made the sound play synchronously, but that doesn't fit well with BabySmash's free-form maniacal button pressing.

Larry suggested I copy the WAV files to a temporary location so they'd be "pinned" down, as there wasn't really a good way to pin these in memory that either of us could come up with. Here's what I did. I grabbed a TempFileName, put the WAV files on disk there and switched the call to PlaySound to the filename overloaded version, rather than the byte[] version. I use TempFileCollection which is helpful because it automatically tries to delete the temporary files when its finalizer runs.

[DllImport("winmm.dll", SetLastError = true)]
static extern bool PlaySound(string pszSound, IntPtr hmod, UInt32 fdwSound);

public void PlayWavResource(string wav)
{
string s = GetWavResource(wav);
PlaySound(s, IntPtr.Zero, SND_ASYNC);
}

public void PlayWavResourceYield(string wav)
{
string s = GetWavResource(wav);
PlaySound(s, IntPtr.Zero, SND_ASYNC | SND_NOSTOP);
}

TempFileCollection tempFiles = new TempFileCollection();

private string GetWavResource(string wav)
{
//TODO: Is this valid double-check caching?
string retVal = null;
if (cachedWavs.ContainsKey(wav))
retVal = cachedWavs[wav];
if (retVal == null)
{
lock (cachedWavsLock)
{
// get the namespace
string strNameSpace = Assembly.GetExecutingAssembly().GetName().Name;

// get the resource into a stream
using (Stream str = Assembly.GetExecutingAssembly().GetManifestResourceStream(strNameSpace + wav))
{
string tempfile = System.IO.Path.GetTempFileName();
tempFiles.AddFile(tempfile,false);
var bStr = new Byte[str.Length];
str.Read(bStr, 0, (int)str.Length);
File.WriteAllBytes(tempfile, bStr);
cachedWavs.Add(wav, tempfile);
return tempfile;
}
}
}
return retVal;
}

It's coarse, but it works, and now I can move on to some cleanup with this bug behind me. The Back to Basics lesson for me is:

  • Don't forget there is a Garbage Collector out there, doing just that.
    • It's easy to forget all about it, but it's so important to know who has a finger on your memory when you're moving back and forth over the unmanaged/managed boundary.
  • Edge cases are no fun.
    • There are always edge cases, race conditions and deadlocks to be found, and I'm sure I've got more left to find! (notice my lack of surety around the lock() call in the comments?)
    • Know your patterns for best practices or, better yet, know where to go to find the answers.
  • Your software typically runs exactly as you wrote it.
    • Even though this was a GC doing something I didn't expect, it was doing its job with the code I provided it. Given how I was using the byte array, it's very deterministic in its behavior.
  • Know what's going wrong before you try to fix it.
    • Once I understood the bug, I was able to reproduce the bug much more easily. "Flakies" are scary, but Bugs are just bugs. If I tried to fix it without understanding it, I'd just be programming by coincidence. (And I may still be! That's the rub.)

Have a nice day!

Technorati Tags: ,,

About Scott

Scott Hanselman is a former professor, former Chief Architect in finance, now speaker, consultant, father, diabetic, and Microsoft employee. He is a failed stand-up comic, a cornrower, and a book author.

facebook twitter subscribe
About   Newsletter
Hosting By
Hosted in an Azure App Service
July 11, 2008 5:03
"If I tried to fix it without understanding it, I'd just be programming by coincidence."

Or alternatively: "Pinball Programming" (via http://badprogrammer.infogami.com)
July 11, 2008 5:21
Have you thought about using unsafe{} and fixed{}?
July 11, 2008 5:30
Fixed still stays in method scope, no? PlaySound actually starts a NEW thread, and by then, the C# method would have ended.
July 11, 2008 5:55
Now you've got me thinking. Hmm. PlaySound takes an LPCTSTR. Sure, the string works, but what if... by astronomical odds... the GC cleans up the heap during a massive context switch while Outlook hits a send/receive cycle and the clock is adjusting for daylight savings time? Huh? What now? The only safe way to ensure that the resource is available when PlaySound wants to mov EDX is with a threadpool thread.
July 11, 2008 6:04
Didn't know about the TempFileCollection, cool! But why on earth is it hidden in System.CodeDom.Compiler??

Does anyone know of a way to play sounds async without writing them to disk first?

Oh, and i noticed you do
Stream str = Assembly.GetExecutingAssembly().GetManifestResourceStream(strNameSpace + wav)
Why don't you put all the sounds in an embedded .resx file? Then you can do TheResourceFile.ResourceManager.GetStream(wav), and you lose the namespace thingy.
July 11, 2008 6:05
Seriously, the string works. I'm no help - I'm typing this on Fedora 9. malloc() and free() would work.
July 11, 2008 6:07
What's up with P/Invoke?

System.Media.SoundPlayer was introduced in framework 2.0.

(http://msdn.microsoft.com/en-us/library/system.media.soundplayer(VS.80).aspx)

You could set the Stream property to your manifest resource stream, use either the LoadAsync() or Load() methods, then use the Play(), PlayLooping(), PlaySync().

(or did I miss something?)

July 11, 2008 6:20
Bill - For some reason, when I use SoundPlayer, I get clipped sound, random crashes and general sadness. My very first BabySmash prototype used it.

How about you switch it back (code is at CodePlex) and let me know? ;)
July 11, 2008 6:34
OK, but I'll have to catch up a bit on this project first because I have not really been following the baby-smash thing, although I've seen a few posts/podcast dicsussions.

BTW, those babies turn into teenagers (eventually), which is kind of where I'm at now. So, I'll have to find time between battles. When mine were little, people would say to me "enjoy them while you can". Now I know what they were getting at.

Maybe I'll do "teen smash" where I actually play the game and try to stop drinking, drugs, sex, etc., and the game elements just tell me I'm stupid, don't know nothing, and I "just need to chill out".

So enjoy it while you can!
July 11, 2008 6:40
Heh, it's more about learning WPF than it is about Babies. ;) You can get the code at http://www.codeplex.com/babysmash and have at it! I think Teen Smash could be a hit. ;)
July 11, 2008 6:46
Just curious, do we know it's safe to ignore the return value of Stream.Read (total number of bytes actually read) on line 37?
July 11, 2008 7:33
Do you consider using a MemoryStream as the cached item?

As I recall, they create/hold an internal byte[] that you can access.

This would give code that looked something like this:


static Dictionary<string, MemoryStream> cachedWavs = new Dictionary<string, MemoryStream>();


public static void PlayWavResourceYield(string wav)
{
MemoryStream str = GetWavResource(wav);
PlaySound(str.GetBuffer(), IntPtr.Zero, SND_ASYNC | SND_MEMORY | SND_NOSTOP);
}
  
private static byte[] GetWavResource(string wav)
{
//TODO: Is this valid double-check caching?
byte[] b = null;
if (cachedWavs.ContainsKey(wav))
b = cachedWavs[wav];
if (b == null)
{
lock (cachedWavsLock)
{
// test if the item has been adding while we waited for lock
if (cachedWavs.ContainsKey(wav))
return cachedWavs[wav];


// get the namespace
string strNameSpace = Assembly.GetExecutingAssembly().GetName().Name;

// get the resource into a stream
using (Stream str = Assembly.GetExecutingAssembly().GetManifestResourceStream(strNameSpace + wav))
{
if (str == null)
throw new ArgumentException(wav + " not found!");
// bring stream into a byte array
var bStr = new Byte[str.Length];
str.Read(bStr, 0, (int)str.Length);

// only some of the MemoryStream Ctors allow us to call GetBuffer...
ms = new MemoryStream(bStr.Length);
ms.Write(bStr, 0, bStr.Length);

cachedWavs.Add(wav, ms);
return ms;
}
}
}
return b;
}


As far as I remember, this should handle the byte[] lifetime correctly.
July 11, 2008 7:40
Crap, there are syntax errors in my code sample...oh well, it compiled fine in the text box when I was typing it...and I put the pre tag too far down.

I guess its time to go to sleep now...
July 11, 2008 9:18
You could also use the WeakReference within your cache to help determine if your reference was being GC'ed (i.e., the IsAlive property).
July 11, 2008 9:40
Generally, I haven't had a good look at what you're doing here, but wouldn't a memory stream save the disk access?
July 11, 2008 10:15
Nick - Good idea!

Chris - Our belief is that the byte[] is being GC'ed as PlaySound() starts up a new thread and my method leaves. When memory pressure causes a GC, my byte[] is tossed in the middle, or slightly before, the PlaySound thread gets started. I'll try the MemoryStream, but I think it could be GC'ed also, although Jacco's idea is a good one.

AdamR - Probably not, but considering that the stream is coming from my own assembly, if it didn't work, I'm in bigger trouble than I can handle. I could add an Assert or try/cach.
July 11, 2008 10:22
Whatever approach you take if the byte[] is in memory instead of being loaded from disk you will have to use a GCHandle to pin the object. If you don't the GC is allowed to move that memory to another location while the unmanaged code is still attempting to access it. In both examples (Scott's original code and the code using 'Dictionary<string, MemoryStream>') the byte[] array will only be fixed during the call to PlaySound. I.e.

public static void PlayWavResourceYield(string wav)
{
byte[] b = GetWavResource(wav);
PlaySound(b, IntPtr.Zero, SND_ASYNC | SND_MEMORY | SND_NOSTOP); // Only fixed during this call
}
July 11, 2008 11:49
Luke - Right, and that call, because I pass in SND_ASYNC returns *immediately* which causes the byte[] to leave scope and opens the real possibility that it gets GC'ed. Sounds like in-memory byte[]'s and the Win32 PlaySound API just don't match well when SND_ASYNC is used. I'll talk to the owners (LarryO) and the owner of System.Media.SoundPlayer and see what they say.
July 11, 2008 11:52
Bill - Take a look in Reflector for the details on the managed SoundPlayer class. It does a whole bunch of validation on the WAV stream, but it's just a wrapper also. Looks like mine may be a lot faster for my (odd) scenario because I was constructing and calling it dozens of times, which would have caused the byte[] validation to happen all the time. However, from what I can read, it would still have the same problem with SND_ASYNC, which is what I'll bring up with the team.
July 11, 2008 11:57
Ah, I see now, SoundPlayer has a private byte[] in its class instance so as long as the SoundPlayer itself (and its members) doesn't get GC (meaning you hang on to a SoundPlayer for longer than it plays) than its Play should work Asynchronously. My trouble with it must have been a combination of the creation/deletion of a SoundPlayer along with the repetitive validation code. Still, I'll ask the dev who wrote it.

Fun stuff!
July 11, 2008 12:27
Scott; Have you considered using Static Code analysis on the code? I'd definitely recommend it as it can fix quite a few design and performance issues. For example in the code discussed in this post we'd be better of enclosing the dllimport code in a separate class called "SafeNativeMethods"; this in accordance to rule CA1060 (link: http://www.bokebb.com/dev/english/1940/posts/194013439.shtml).
July 11, 2008 13:39
I don't get it...

byte may be a value type, but byte[] isn't. So if every byte[] you pass to PlaySound is also stored in cachedWavs, then how can it be garbage collected? cachedWavs still has a reference to the same instance that PlaySound uses! So unless cachedWavs is not garbage collected, neither should the byte[].

Can someone please explain to me where I'm wrong?

Thanks!
July 11, 2008 14:13
The point Luke's making is not that the byte[] may get collected, but that it may get moved during collection. The unmanaged code doesn't know about the move, as it's accessing it via a raw pointer rather than a GCHandle.

The most direct way to prevent this is to cache a pinned GCHandle to the object, created like this:
GCHandle wavHandle = GCHandle.Alloc(b, GCHandleType.Pinned);


Then pass wavHandle.AddrOfPinnedObject to PlaySound.

The other problem the incorrect double-guard in the caching. In the original code one thread could be preempted while creating the wav, allowing another to enter the method and pass through the ContainsKey check. It will stop at the lock statement, but would then continue once the original thread leaves the lock block. The second thread would recreate the wav and update the dictionary with it, causing an exception due to duplicate entries with the same key.

Also, use Dictionary.TryGetValue() to avoid looking up the dictionary twice.


byte[] b;
if ( ! cachedWave.TryGetValue(wav, out b) )
{
lock ( cachedWavsLock )
{
if ( ! cachedWavs.TryGetValue(wav, out b) )
{
b = new ...
...
cachedWavs.Add(wav, b);
}
}
}

return b;

July 11, 2008 14:43
Thanks Luke & Blake, I think I get it now. The byte[] is indeed not collected as Scott's post suggests, but merely moved arround in memory. However I wasn't really aware of the fact that the GC does that either. Then again I'm not doing much P/Invoke stuff in my code anyway.

If I understand correctly another solution for the problem would be to spin up a worker thread in managed code and then call PlaySound synchronously in this worker thread, right?
July 11, 2008 14:53
Creating a whole bunch of SoundPlayer objects (System.Media.SoundPlayer) seems to work OK. Without using reflector, I assumed this object was a "wrapper" so I was thinking - "not much different than the current implementation", and not too concerned that approach would be heavy-weight/bloat (store SoundPlayers in the dictionary instead of Wav's). But what advantage would this be? Maybe just to assume the SoundPlayer implementation delt with the data buffer better. Maybe.

Getting back to the current approach (P/Invoke), one idea would be to put the WAV data in unmanaged memory. I am not too familiar with the GC (pinning and all), but I am with P/Invoke and the Marshal class. You could allocate unmanaged memory, copy over there, and remember to free it later. The only thing I like about this idea is that if we are going to use P/Invoke to call the (potentially Async) windows API, then it seems to be natural that the data buffer be "over there" in unmanaged land as well.

July 11, 2008 15:06
What Blake said. Or get a pointer to a chunk of unmanaged memory with (System.Runtime.InteropServices) Marshal.AllocHGlobal() (remembering to FreeHGlobal when you're done), copy your bytes over using Marshal.Copy(). Not sure what the code access security implications would be for your scenario though.
July 11, 2008 15:29
Regarding proper cache access, I just came across this link (http://blogs.microsoft.co.il/blogs/sasha/archive/2008/07/11/practical-concurrency-patterns-read-write-cache.aspx), which had a great post on properly creating a thread-safe cache.
July 11, 2008 16:15
Looks like I fell into a common trap. Hashtable is thread safe for multiple readers and one writer, but Dictionary<T,V> is not. That's pretty frustrating.

Here's some more info:
http://blogs.msdn.com/kimhamil/archive/2008/03/08/hashtable-and-dictionary-thread-safety-considerations.aspx
July 11, 2008 17:54
It's been a while since I used the CLR (haven't touched it since I graduated), but wasn't there a way to explicitly reference and pin memory in the GC for exactly this sort of thing? Or does the asynchronous playsound function not have a completion callback for you to dereference and unpin the memory?
July 11, 2008 18:06
Have you tried using SoundPlayerAction in a trigger?
July 11, 2008 18:51
Nice discussion about the garbae collection and all, but rather than write to file, why not just do the async yourself? .NET has excellent async support.

You could just use the synchronous version of PlaySound, wrap it in a delegate and call BeingInvoke(); TA DA! No writing sounds to disk!
July 11, 2008 20:04
Btw, the PlaySound API copies the filename parameter to a local buffer when used with the SND_ASYNC operation - that way you can have:

foo()
{
char buffer[100] = "ding.wav";
PlaySound(buffer, NULL, SND_ASYNC);
}

One other suggestion I made to Scott to avoid the temporary file thingy is to use SND_RESOURCE and specify a resource ID and module handle for the first and second parameters to the function.

July 11, 2008 23:22
Asynchronous or not, the byte arrays shouldn't be getting GC'd if they have strong references in your cache. In fact, if they were, things like this or this wouldn't happen.

I'll have to take a look at the code later tonight to see if there's something about the cache in your app that you haven't told us.

The only thing I can think might cause this is if your cache is just pointing to the space created during GetManifestResourceStream (which doesn't appear to be the case because Stream.Read should copy the data into the byte array) and the GC disregards that reference and collects anyway. Or if the ASP.NET cache is somehow exempt from proper GC.
July 12, 2008 2:23
I love your app but I have one request. Please oh please can you do a non click once deployment release?
I want to put this on a laptop without an internet connection and I dont want to connect it to the net either....... please?
Tom
July 14, 2008 18:38
Byte may be a value type, but arrays (even arrays of value types) are reference types, and what you are creating, caching, and sending to the PlaySound API is an array, which means that there is no way that the GC is actually reclaiming the memory while the sound is playing. Unless I am missing something.

However, it IS possible that the GC is moving the array in managed memory, which could potentially have the same result. Has anyone tried the suggestion of using GCHandle and/or unmanaged memory? If not I might give it a try.
July 15, 2008 18:01
@Scott, Blake pointed out the problem in your double-check cache, but as an alternative, you should be able toomit the lock completely and add the item to the cache using cache[key]=value instead of cache.add(key,value) semantics. The advantage of the [] approach is that you'll simply replace the etry if it's already there, so you don't have to worry about a key collision.

@Jay, the problem with rolling your own is that the async support relies on the threadpool, and there are a limited number of threads in the pool. With dozens of invokes a second you may very well eat the pool.

-Walden

Comments are closed.

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.