Android backend input thread block fix

[GoogleCodeExporter]

As mentioned in the forum:
http://www.badlogicgames.com/forum/viewtopic.php?f=11&t=100&p=552#p552

Original issue reported on code.google.com by lf3th...@gmail.com on 13 Nov 2010 at 7:30

Attachments:

• AndroidNonblockingInputEvent.patch

[GoogleCodeExporter]

Ok, here's a couple of things. I can't apply that patch just now due to a 
couple of reasons. All my line references are relative to this link 
http://code.google.com/p/libgdx/issues/attachmentText?id=60&aid=3970375173295234
026&name=AndroidNonblockingInputEvent.patch&token=b4291cd2bea0dd06316128c54121f6
cd.

- line 72, 73: i guess that's been overlooked by you. no biggie
- line 83: for each creates an iterator. this is a no go as the gc will 
complain about 4 seconds in 
- line 97: see above
- line 181: could you explain why you are allocating a new KeyEvent in this 
case? You do it in any case, no pooling of instances. That will make the gc 
complain again after a couple of 100 keyevents (yeah, it does not operate on 
memory pressure...)
- i don't cite any other lines where you allocate new XXXEvent instances. This 
is not good

that's what's in the patch. Now the patch uses ConcurrentLinkedList. That is a 
good idea on the desktop but a bad idea on Android. Android uses the Harmony 
implementation of most Java SE classes. Here's the first few lines of the 
CLL.offer() method:

 public boolean offer(E o) {
        if (o == null) throw new NullPointerException();
        Node<E> n = new Node<E>(o, null);
        for(;;) {

allocation, there's a couple more of those in AbstractQueue which 
ConcurrentLinkedList derrives from. 

I hope you can understand that i can't apply this patch as it is now. Memory 
allocation is a no go.

Please don't missunderstand this, your contribution is welcome! We all want to 
make it a better lib. This just doesn't quite cut it.

Original comment by badlogicgames on 13 Nov 2010 at 11:57

[GoogleCodeExporter]

Original comment by badlogicgames on 13 Nov 2010 at 11:57

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

My bad, line 83 is actually my crappy code. Fixed in SVN. The rest still holds 
though.

Original comment by badlogicgames on 14 Nov 2010 at 12:00

[GoogleCodeExporter]

No biggies, always good to have someone more knowledgeable in Java to 
scrutinize my code. ;-)
line 181: I'm only allocating when I cannot find any unused KeyEvent from the 
pool. The event pool is replenished when processEvents() is called. So it's 
actually doing a pool system only that it's outside and a little hackish.

Hmm.. since the ConcurrentLinkedList internal code does a new all the time, 
let's ditch this and make our own LockLessQueue pooling class. I'll come up 
with something and submit a new patch. ;-) Didn't know the implementation sucks 
that bad in Android. That calls for more self written and better optimized 
internal algorithms. :-)

Original comment by lf3th...@gmail.com on 14 Nov 2010 at 1:16

[GoogleCodeExporter]

Ok. Here's a new attempt.

I've added a new class template in utils called LocklessThreadQueue. It's 
basically a fixed sized preallocated pooled single directional lockess thread 
queue. What this means is that it will initialize with a preallocated pool 
buffer of queue items. The restriction of using this is that only one thread is 
allowed to do the pushing and one thread is allowed to do the popping. Other 
than that, this should be very fast and clean for GC since no allocation is 
done during the queuing process.

However, being a fixed sized queue, it means there is a chance that the queue 
list will be packed full and unable to push new events in.

Still, this works pretty well on touch events since for the most part, we can 
simply discard the events while the graphics thread is busy doing some heavy 
processing. The exception is the touch up event which must get through if a 
touch down event went through before. If not, the touch up event will get lost 
and will cause bad touch state consistency. For that case, I've simply do a 
while loop until the graphics thread clears up the queue (which will hog the 
main thread. but I think when it comes to this case, it shouldn't matter since 
graphics thread is lagging anyways which is already bad).

I've tested this on the android phone I borrowed and it works like a charm. 
Responsive input all the way from the start. :-D

Tell me what you think. ;-)

Original comment by lf3th...@gmail.com on 14 Nov 2010 at 9:17

Attachments:

• locklessInputQueuePatch.zip

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Hi, 

I can't apply the patch to the current trunk as the files don't match anymore. 
I saw this post to late or else i'd have merged it 15 hours ago. Could you 
update to the latest trunk and create a new patch?

Thanks,
Mario

Original comment by badlogicgames on 15 Nov 2010 at 4:13

[GoogleCodeExporter]

I couldn't apply the patch yet but i quickly glanced over it. There's a couple 
of these in there:

while ((event = keyEvents.prepare()) == null);

That's basically a spin lock. Why is that any better than a synchronization 
block where the thread that waits for the mutex sleeps and gives CPU time to 
all other thrads?

Also, could your provide me with the test case where this patch fixes the input 
lag? Do you have lag issues with any of the tests in libgdx itself, e.g. 
MultiTouchTest?

Original comment by badlogicgames on 15 Nov 2010 at 4:25

[GoogleCodeExporter]

Hi, I'll try and update a new patch for this, About that spin lock, was just a 
quick hack to test functionality. I could add a wait() notify() method instead 
if you like. I'll try to come up with a test that shows the issue though I'm 
not sure exactly how since the issue had been simply input synchronize locked 
up while waiting for rendering thread's synchronize which happened very often 
if you start clicking on the screen right when the app runs.

Oh, btw, I found some more bugs on Scene2D.Group for hit testing with scaled 
Actors. Will submit another patch soon.

Original comment by lf3th...@gmail.com on 15 Nov 2010 at 9:32

[GoogleCodeExporter]

I fixed a corresponding bug yesterday in Group as pointed out by someone in the 
forums. Group.toChildCoordinates() had a bogus line in there. 

Original comment by badlogicgames on 15 Nov 2010 at 10:02

[GoogleCodeExporter]

Yes, I noticed. But it was still wrong. Here's a patch.
http://code.google.com/p/libgdx/issues/detail?id=62

Original comment by lf3th...@gmail.com on 15 Nov 2010 at 10:22

[GoogleCodeExporter]

Thanks, it's in SVN now. Let's stay on track with this issue here :)

Do you have a test case with which i can reliably reproduce the input lag? 

Original comment by badlogicgames on 15 Nov 2010 at 3:48

[GoogleCodeExporter]

No prob. Yes let's stay on track with this one. :)

I don't have a test for this since the issue was found in my game. It was 
lagging even in the main menu. But what I suspect is that it's due to the 
synchronization lock during the input dispatch. Probably when input dispatch 
does some heavy processing caused the main thread to fail to enter a lock for 
update.

I need to revert my changes to test again and see if I can get it to happen on 
my testing phone. I'll use your input test module as a base.

Original comment by lf3th...@gmail.com on 15 Nov 2010 at 4:46

[GoogleCodeExporter]

Ok, i think i've seen the light. Here's the thing:

as you noticed i have to lock the complete drawFrame() method each frame:

synchronized(Gdx.input) {
 ... 
 listener.render();
 Gdx.input.processEvents(null);
}

The second processEvents is for the case where the user didn't call 
Gdx.input.processEvents() himself. In that case i need to empty the event queue 
or else it grows unbounded (e.g. some of the tests of libgdx don't call 
processEvents()). I need the synch as new events can arrive in between 
listener.render() and Gdx.input.processEvents(null). Otherwise the new events 
that arrived in between will get removed by processEvents() before the next 
render, which means we lose events (e.g. if you remove the synch above and test 
InputTest.java then you'll often lose the touch up event). 

This also means that i lock the whole fucking rendering thread body each frame. 
With some nice timing the UI thread might never have the chance to to get the 
mutex lock.

The processEvents(null) call is also problematic if someone wants to process 
input events from another thread than the rendering thread. In this case events 
will get lost too. 

But i have to clear the buffer in case the user never empties the buffer 
himself. So what would be a solution?

Your lockless patch is one. I want to modify it so that no events get lost. We 
can live with object allocation if things get bogged down and the events aren't 
processed fast enough (you can limit the events per second via the sleepTime 
parameter in the AndroidApplication.initialize() method). 

Another solution (which is the same as the old solution really) is to force the 
user to have an input processor. This way we'd only have a very small critical 
region that needs to get locked and both the UI and render thread would happily 
life besides each other. I could imagine to add a processEvents() method to the 
ApplicationListener interface. It would get called every frame before 
listener.render() for example (or after it).

I still like the other solution better though :/

Also, i can't reproduce the lag issue, even if i do heavy duty work in the 
touch listener and rendering thread (Hero, Droid, Nexus One). It makes me 
crazy. 

Original comment by badlogicgames on 15 Nov 2010 at 7:04

• Changed state: Started

[GoogleCodeExporter]

Lefthand, i just changed the input processing completely. We reverted back to 
the old listener concept so we have the most minimal critical section possible. 
Please try this out for your game before you invest more time in creating that 
patch for the latest version. 

Original comment by badlogicgames on 15 Nov 2010 at 9:52

[GoogleCodeExporter]

Ok, here's a new patch.

1. Changed spin lock to thread wait/notify
2. Moved polling input data update to processEvents so as to not lock up the 
main thread.
3. Moved dropping events into processEvents instead of main event callback. 
This is actually much better since for the most part, Android seems to be 
giving tons of dragged events within a single frame which is useless since only 
the last event would be visible to users in any sort of dragging interactions. 
So we do lock up the main thread if game process thread locks up and runs out 
of event queue slots.
4. Added an image drawing base on touch to better visualize input events in the 
InputTest. My patch with the collapsing/dropping of move events actually helped 
a lot here and made input feels more responsive since there's less events 
callback.
5. Fixed a minor bug on LocklessThreadQueue to use one less space for it's 
queue buffer.

One issue to note is that setInputProcessor() cannot be set to null within an 
input callback or it might cause a null pointer assertion.

Btw, I can't reproduce the input lag I mentioned anymore. Most likely your 
previous fix fixed it. :-) Still, I think this new lockless method is better 
and allows for more performance and CPU time in the rendering thread.

Original comment by lf3th...@gmail.com on 16 Nov 2010 at 9:34

Attachments:

• LocklessInputQueue.patch

[GoogleCodeExporter]

Oh yeah, I should mention that this is base on your changed input processing 
system.

Original comment by lf3th...@gmail.com on 16 Nov 2010 at 10:25

[GoogleCodeExporter]

Cool, you are right about tue setInputProcessor null reference problem, will 
fix it.

concerning your patch i'll give it a try. if it works out we'll keep it :)

Thanks for all the help i really appreciate it!

Original comment by badlogicgames on 16 Nov 2010 at 10:31

[GoogleCodeExporter]

No probs :D Glad I can help. Your lib really saved me when I was looking for a 
way to prototype a game idea for the android OS fast with virtually no 
experience in mobile development.

So contributing to improve the lib is my way of gratitude. :) This is why I 
love open source so much. lol

Anyways, no worries if my patch doesn't fit the bill. We can always fall back 
to your method with the drag event collapsing code and get about the same 
effect I think.

Original comment by lf3th...@gmail.com on 16 Nov 2010 at 10:44

[GoogleCodeExporter]

Yeah, oss ftw, high five :)

I'm sorry if i come of as being a control nazi about including your patch. I 
just need time to review. Version 0.8 already had its share of bugs (altough 
your code looks good at first glance). 

If you don't mind i set this issue to fixed. I'll play around with the lockless 
code tonight. 

Thanks again!

Original comment by badlogicgames on 16 Nov 2010 at 11:03

[GoogleCodeExporter]

Original comment by badlogicgames on 16 Nov 2010 at 11:03

• Changed state: Fixed

[GoogleCodeExporter]

No worries. It's your project. You have the rights to say what should go in and 
what should be left out. :) I do understand your concern since my patch changes 
a rather major part of the input system.

Original comment by lf3th...@gmail.com on 16 Nov 2010 at 1:15

[GoogleCodeExporter]

Bumping here with a new patch as I found a bug with my lock less input code 
causing the main thread to get blocked infinitely when one spams the touch 
input. The reason being that the lockless queue got full and main thread goes 
into a wait in the WRONG context!

Anyways, no need to patch it if you feel it's still not stable. ;) But I'm 
using it for my own project as I found it more responsive when it comes to 
multitouch spamming.

Original comment by lf3th...@gmail.com on 24 Nov 2010 at 9:23

Attachments:

• LocklessInputQueue.patch