Wasn't sure where to put this, but here's a patch to make the static devices stuff in InputHandler thread-safe. There was actually a TODO in the code for this, but I also wanted it for the JMEDesktop stuff I'm still plowing through. I didn't check the entire class, just the static devices stuff because I keep getting sidetracked. JMEDesktop touches so many areas so hopefully my patches won't become annoying.
/**
@@ -238,40 +241,36 @@
* Devices for all handlers.
* TODO: we could decide to have one device per handler to reduce amount of triggers that are checked on each event
*/
- private static Map<String, InputHandlerDevice> devices;
+ private static final ConcurrentMap<String, InputHandlerDevice> devices = new ConcurrentHashMap<String, InputHandlerDevice>();
+ private static boolean devicesInitialized;
+ private static final ReentrantLock devicesInitializedLock = new ReentrantLock();
+
- /**
- * create mouse, keyboard and joystick devices (if not yet created).
- */
- private static void initializeDefaultDevices() {
- //TODO: synchronize if multithreaded creation of handlers should be supported
- if ( devices == null ) {
- devices = new HashMap<String, InputHandlerDevice>();
- addDevice( new MouseInputHandlerDevice() );
- addDevice( new KeyboardInputHandlerDevice() );
- for ( int i = JoystickInput.get().getJoystickCount() - 1; i >= 0; i-- ) {
- Joystick joystick = JoystickInput.get().getJoystick( i );
- String name = joystick.getName();
- String uniqueName = name;
- for ( int j=2; devices.get(uniqueName) != null; j++ ) {
- uniqueName = name + " ("+j+")";
- }
- addDevice( new JoystickInputHandlerDevice( joystick, uniqueName ) );
- }
+ private static Map<String, InputHandlerDevice> getDevicesMap() {
+ devicesInitializedLock.lock();
+ try {
+ if (!devicesInitialized) {
+ devicesInitialized = true;
+ addDevice( new MouseInputHandlerDevice() );
+ addDevice( new KeyboardInputHandlerDevice() );
+ for ( int i = JoystickInput.get().getJoystickCount() - 1; i >= 0; i-- ) {
+ Joystick joystick = JoystickInput.get().getJoystick( i );
+ String name = joystick.getName();
+ String uniqueName = name;
+ for ( int j=2; devices.get(uniqueName) != null; j++ ) {
+ uniqueName = name + " ("+j+")";
+ }
+ addDevice( new JoystickInputHandlerDevice( joystick, uniqueName ) );
+ }
+ }
+ } finally {
+ devicesInitializedLock.unlock();
}
+ return devices;
}
-
- private static Collection<InputHandlerDevice> devicesReadOnly;
-
+
public static Collection<InputHandlerDevice> getDevices() {
- if ( devices == null ) {
- throw new IllegalStateException( "devices have not been initialized" +
- " - create an InputHandler first" );
- }
- if ( devicesReadOnly == null ) {
- devicesReadOnly = Collections.unmodifiableCollection( devices.values() );
- }
- return devicesReadOnly;
+ return Collections.unmodifiableCollection( getDevicesMap().values() );
}
/**
@@ -308,13 +307,14 @@
* @param allowRepeats false to invoke action once for each button down, true to invoke each frame while the
*/
public void addAction( InputActionInterface action, String deviceName, int button, int axis, boolean allowRepeats ) {
+ Map<String, InputHandlerDevice> devicesMap = getDevicesMap();
if ( DEVICE_ALL.equals( deviceName ) ) {
- for ( InputHandlerDevice device : devices.values() ) {
+ for ( InputHandlerDevice device : devicesMap.values() ) {
device.createTriggers( action, axis, button, allowRepeats, this );
}
}
else {
- InputHandlerDevice device = devices.get( deviceName );
+ InputHandlerDevice device = devicesMap.get( deviceName );
if ( device != null ) {
device.createTriggers( action, axis, button, allowRepeats, this );
}
I'll be happy to test this out and if it works on the tests, I'll apply it,… but I'm not expert in this area, so I'm going to be trusting you a bit here. Someone else bark if they take issue with the results.
In svn. The code changes made sense to me and it tested fine against a wide selection of jmetests on my windows machine. Others please test and report any issues.
And thanks a bunch for the work in this area, lazlohf.
Yeah seriously. Every class I look at has Renanse's name on it… I half-expect to open some random java source for a class like String and find his name there…
Sorry to be in here that late, but I have a note about the patch: It creates garbage for each InputHandler each frame now and it's slower than before
Why do you think using the fields was bad? Calling a method each time and esp. creating a new unmodifiable collection each time does not seem reasonable to me… why not just synchronize initializeDefaultDevices plus using a concurrent hash map (still slower than the original implementation but ok, I guess)?
Collections.unmodifiableCollection creates a new collection wrapper each time it is called. But I was mistaken about the "each frame". It's only when you add an action - which is not that problematic. Still why shouldn't the unmodifiable collection be still stored in that attribute?
Sorry didn't see this until today, is anyone else not getting email notifications for forum replies?
irrisor said:
Sorry to be in here that late, but I have a note about the patch: It creates garbage for each InputHandler each frame now and it's slower than before :|
Overall it may be a few cycles slower but the device list access is now thread-safe. Someone might notice a slowdown if they were adding lots of actions each frame.
irrisor said:
Why do you think using the fields was bad? Calling a method each time and esp. creating a new unmodifiable collection each time does not seem reasonable to me... why not just synchronize initializeDefaultDevices plus using a concurrent hash map (still slower than the original implementation but ok, I guess)?
The devices stuff is public static, so it was accessible without a call to initializeDefaultDevices through the InputHandler constructor. I just thought it was clearer and less likely to bite someone later if the lazy instantiation was moved into the static getter. It also eliminated the IllegalStateException check and having to create an InputHandler first. Also, the call to getDevicesMap() makes sure the default devices are initialized before returning the unmodifiable list.
irrisor said:
Collections.unmodifiableCollection creates a new collection wrapper each time it is called. But I was mistaken about the "each frame". It's only when you add an action - which is not that problematic. Still why shouldn't the unmodifiable collection be still stored in that attribute?
ConcurrentHashMap.values() is backed by the map so yeah you can store it as a private static final instead of creating it. You just need to make sure the initialization section runs before handing it out. I can't do the patch at this moment, but I can make it tonight unless you guys find the time.
Here's an optimization patch to replace the devices lock with the classloader lock and re-use the unmodified collection instead of creating a new one each call. The DeviceMap class and all of it's methods are final so the getters to the internal fields will be inlined by the compiler.
