3
\$\begingroup\$

I am trying to optimize my game written in Java and I used the VisualVM launcher for profiling and monitoring my game. What caught me by surprise was that my basic utility to play sound effects was using the most CPU in my game.

I'd really appreciate some tips on improvements which I can implement, because I really don't think that this basic utility for playing sound effects should utilize that much processing power!

Here is the relevant method for playing audio:

public static synchronized void playSound(final AudioInputStream inputStream, int priority) {
    switch(priority){
    default:
    case LOW_PRIORITY:
        if(nThreads>= MAX_THREADS>>1) return;
        break;
    case NORMAL_PRIORITY:
        if(nThreads>= MAX_THREADS) return;
        break;
    case HIGH_PRIORITY:
        break;
    }
    new Thread(() -> {
        nThreads++;
        try {
            Clip clip = AudioSystem.getClip();
            inputStream.reset();
            clip.open(inputStream);
            clip.start();
            while(clip.getMicrosecondPosition() < clip.getMicrosecondLength());
        } catch (Exception e) {
            e.printStackTrace();
        }
        nThreads--;
    }).start();
}

Just for reference here is the whole class:

package foo.bar.audio;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.net.URL;

import javax.sound.sampled.AudioInputStream;
import javax.sound.sampled.AudioSystem;
import javax.sound.sampled.Clip;
import javax.sound.sampled.UnsupportedAudioFileException;

public class BasicSoundEffect {

    public static final int MAX_THREADS = 20;

    public static final int HIGH_PRIORITY   = 2;
    public static final int NORMAL_PRIORITY = 1;
    public static final int LOW_PRIORITY    = 0;

    public static AudioInputStream SFX_BOSS;
    public static AudioInputStream SFX_HIT;
    public static AudioInputStream SFX_HURT;
    public static AudioInputStream SFX_LOSE;
    public static AudioInputStream SFX_PICKUP;
    public static AudioInputStream SFX_SHOOT;
    public static AudioInputStream SFX_WIN;

    static{
        try {
            SFX_BOSS    = createReusableAudioInputStream(BasicSoundEffect.class.getResource("/boss.wav"));
            SFX_HIT     = createReusableAudioInputStream(BasicSoundEffect.class.getResource("/hit.wav"));
            SFX_HURT    = createReusableAudioInputStream(BasicSoundEffect.class.getResource("/hurt.wav"));
            SFX_LOSE    = createReusableAudioInputStream(BasicSoundEffect.class.getResource("/lose.wav"));
            SFX_PICKUP  = createReusableAudioInputStream(BasicSoundEffect.class.getResource("/pickup.wav"));
            SFX_SHOOT   = createReusableAudioInputStream(BasicSoundEffect.class.getResource("/shoot.wav"));
            SFX_WIN     = createReusableAudioInputStream(BasicSoundEffect.class.getResource("/win.wav"));
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    private static AudioInputStream createReusableAudioInputStream(URL url) 
            throws IOException, UnsupportedAudioFileException
        {
            AudioInputStream ais = null;
            try
            {
                ais = AudioSystem.getAudioInputStream(url);
                byte[] buffer = new byte[1024 * 32];
                int read = 0;
                ByteArrayOutputStream baos = 
                    new ByteArrayOutputStream(buffer.length);
                while ((read = ais.read(buffer, 0, buffer.length)) != -1)
                {
                    baos.write(buffer, 0, read);
                }
                AudioInputStream reusableAis = 
                    new AudioInputStream(
                            new ByteArrayInputStream(baos.toByteArray()),
                            ais.getFormat(),
                            AudioSystem.NOT_SPECIFIED);
                return reusableAis;
            }
            finally
            {
                if (ais != null)
                {
                    ais.close();
                }
            }
        }

    private static volatile int nThreads = 0;

    public static synchronized void playSound(final AudioInputStream inputStream, int priority) {
        switch(priority){
        default:
        case LOW_PRIORITY:
            if(nThreads>= MAX_THREADS>>1) return;
            break;
        case NORMAL_PRIORITY:
            if(nThreads>= MAX_THREADS) return;
            break;
        case HIGH_PRIORITY:
            break;
        }
        new Thread(() -> {
            nThreads++;
            try {
                Clip clip = AudioSystem.getClip();
                inputStream.reset();
                clip.open(inputStream);
                clip.start();
                while(clip.getMicrosecondPosition() < clip.getMicrosecondLength());
            } catch (Exception e) {
                e.printStackTrace();
            }
            nThreads--;
        }).start();
    }
}
\$\endgroup\$
  • \$\begingroup\$ what is nThreads ? \$\endgroup\$ – rolfl Jun 12 '15 at 14:33
  • \$\begingroup\$ @rolfl private static volatile int nThreads = 0; I included the whole class below the method code above. \$\endgroup\$ – Linus Jun 12 '15 at 14:35
  • \$\begingroup\$ Drat, so you did.... ;) \$\endgroup\$ – rolfl Jun 12 '15 at 14:36
2
\$\begingroup\$

Using volatile is no longer a recommended way of handling memory access. A much better way would be to use an AtomicInteger. It has increment and decrement methods to help.

Additionally, you still have race condition in your method even though it is synchronized.... you could have a situation where the nThreads changes (up or down) during execution of the playSound method because the Runnable is running in a different method.

To avoid the race condition, just pull the nThreads in to a local variable once and reference that.

As for your performance problem, it is here:

            while(clip.getMicrosecondPosition() < clip.getMicrosecondLength());

That code.... is horrible... really horrible.

What you do is spin-loop with a CPU at 100% utilization until the sound finishes. You need to do some blocking (or sleeping) until the sound completes.

I would rewrite that loop, and the nThreads something like (note, you will have to handle an InterruptedException somewhere):

AtomicInteger nThreads = new AtomicInteger(0);

// note, not synchronized
public static void playSound(final AudioInputStream inputStream, int priority) {
    final int tcount = nThreads.get();
    switch(priority) {
    default:
    case LOW_PRIORITY:
        if(tcount >= MAX_THREADS >> 1) {
            return;
        }
        break;
    case NORMAL_PRIORITY:
        if(tcount >= MAX_THREADS) {
            return;
        }
        break;
    case HIGH_PRIORITY:
        break;
    }

    new Thread(() -> {
        nThreads.incrementAndGet();
        try {
            Clip clip = AudioSystem.getClip();
            inputStream.reset();
            clip.open(inputStream);
            clip.start();

            final long length = clip.getMicrosecondLength();
            long remaining = length - clip.getMicrosecondPosition(); 
            while(remaining > 0) {
                Thread.sleep(remaining);
                remaining = length - clip.getMicrosecondPosition();
            }
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            nThreads.decrementAndGet();
        }
    }).start();
}
\$\endgroup\$
  • \$\begingroup\$ Thank you, I figured it was because of that while loop but I didn't know an alternative way of doing it. Just a question, why is using the keyword volatile deprecated? \$\endgroup\$ – Linus Jun 12 '15 at 15:13
  • \$\begingroup\$ also I assume long remaining = length - clip.getMicrosecondPosition() < should be long remaining = length - clip.getMicrosecondPosition(); \$\endgroup\$ – Linus Jun 12 '15 at 15:21
  • \$\begingroup\$ You're right about the remaining... I never tested the code. I will fix it. About volatile - mixing different memory management techniques is a problem, also, your code is not thread-safe with volatile anyway ( nThreads++ has race conditions: stackoverflow.com/questions/7805192/… ) \$\endgroup\$ – rolfl Jun 12 '15 at 15:23
  • \$\begingroup\$ Erm, I am sorry to bother you again but Thread.sleep(remaining) would not work because it will sleep for n milliseconds, however we're dealing with microseconds, right? \$\endgroup\$ – Linus Jun 12 '15 at 15:34
  • \$\begingroup\$ Cough yes... you can use the Thread.sleep(millis,nanos) version. Note that you will not likely need microsecond accuracy, and rounding up to the millisecond will be fine, right? \$\endgroup\$ – rolfl Jun 12 '15 at 15:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.