thread-driven changes of input

[Max mtg]

I have added that time filter to playArea, check if the issue with counters is solved.

[Max mtg]

I don't want to work on UX right now, because the primary thing to test were crashes/exceptions when playing spells with exotic costs or triggers on phase/zone changes.

It is fine to focus on other bugs for now, but the branch should not be merged until UX (user experience, for those not familiar with the terminology) issues are resolved. This change has a heavy impact on UX, and these are important UX regressions to resolve before the code is merged.

Disagreed. UX issues may be moved to trunk, taking into consideration there are 2 weeks before next beta to fix them. The flashing buttons - cannot call them causing 'heavy impact on UX'.

Pictures in VAssignDamage - resolved by moving call to EDT. Must have been broken because of forge/view/arcane/CardPanel.java:647 that probably evaluated to false when run from other thread than edt. (it didn't reach ImageCache.getImage)

This points to crossover between classes that operate with threads and classes that interact with GUI widgets, which is worrying. Do we have architecture in place to enforce the requirement that no GUI widgets are interacted with in any way from non-EDT threads? Swing is not thread safe (except for a very few specific method calls), and we need to be very careful about keeping threads away from any swing-related classes.

At this point there is no problem - the VAssignDamage class is completelly operated from EDT. see diff for 20608.

It's impossible to "enforce the requirement". Over 50% of resolve() methods of effects call GuiChoose.**** from non-EDT thread. These threads are not supposed to be concurrent right now. One of threads or EDT executes, the rest (if any) await on a CountDownLatch.

[myk]

I have added that time filter to playArea, check if the issue with counters is solved.

that part looks good. it looks like there is a similar issue with alpha strike, though. the cards become tapped slowly and unevenly.

It is fine to focus on other bugs for now, but the branch should not be merged until UX [...] issues are resolved. This change has a heavy impact on UX, and these are important UX regressions to resolve before the code is merged.

Disagreed. UX issues may be moved to trunk, taking into consideration there are 2 weeks before next beta to fix them.

That does not describe the path to a stable codebase. What is your hurry? Take the time and do it right. We turned up a number of regressions within a few minutes of simple testing. More devs can test it in a controlled environment within a few days. Why cause a bunch of bug reports in the snapshots thread when we can likely solve 80% of the issues here?

Do we have architecture in place to enforce the requirement that no GUI widgets are interacted with in any way from non-EDT threads?

It's impossible to "enforce the requirement". Over 50% of resolve() methods of effects call GuiChoose.**** from non-EDT thread. These threads are not supposed to be concurrent right now. One of threads or EDT executes, the rest (if any) await on a CountDownLatch.

Java is not C. In Java, it is not sufficient that only one thread is executing at a time. It must be the correct thread. Otherwise you end up with cache coherency problems (some hardware platforms are more prone to this than others, but you can bet that even if you don't run into them, the users will).

[friarsol]

I definitely agree that these changes have made forge feel very jittery. I'll try to setup some decks tomorrow night with a range of different inputs, to test for specific crashes/issues unrelated to UX.

[Max mtg]

Myk, you mix up things: stable is about have no crashes (or exceptions in our case), pleasant interface is not about stability.

> What is your hurry? Take the time and do it right.
Now it would make sense, when there's some activity in this thread.


And I am disappointed that you call "regression" what noone here was able to implement for a few years. I mean remove paid/unpaid commands from inputs, magicstack and upkeep classes, make it possible to use the same codepaths to pay costs during cast and during ability resoves, use an universal input to pick cards for any purpose (intead of dedicated classes for reveal, discard, sacrifice, etc.). So that developers won't have to figure out 'where the code would continue when the input returns' - now it will continue right after the method that showed the input.

The what you see in UI are certainly side effects of these changes. But it looks like noone the very progress which was the primary aim of this project came unnoticed.

[Max mtg]

Java is not C. In Java, it is not sufficient that only one thread is executing at a time. It must be the correct thread. Otherwise you end up with cache coherency problems (some hardware platforms are more prone to this than others, but you can bet that even if you don't run into them, the users will).

Why would you compare Java with C? So are there any 'incorrect' threads in Forge?

About threads correct view on shared memory - for inputs CountDownLatch is in charge to ensure the propper order of events.
http://docs.oracle.com/javase/6/docs/ap ... Visibility

Actions prior to "releasing" synchronizer methods such as Lock.unlock, Semaphore.release, and CountDownLatch.countDown happen-before actions subsequent to a successful "acquiring" method such as Lock.lock, Semaphore.acquire, Condition.await, and CountDownLatch.await on the same synchronizer object in another thread.

In other parts of code (where forge.FThreads.invokeInNewThread(Runnable, boolean) is used) the EDT caller returns soon after starting a new thread and must not change the game state.

[moomarc]

Myk, you mix up things: stable is about have no crashes (or exceptions in our case), pleasant interface is not about stability.

And I am disappointed that you call "regression" what noone here was able to implement for a few years. I mean remove paid/unpaid commands from inputs, magicstack and upkeep classes, make it possible to use the same codepaths to pay costs during cast and during ability resoves, use an universal input to pick cards for any purpose (intead of dedicated classes for reveal, discard, sacrifice, etc.). So that developers won't have to figure out 'where the code would continue when the input returns' - now it will continue right after the method that showed the input.

The what you see in UI are certainly side effects of these changes. But it looks like noone the very progress which was the primary aim of this project came unnoticed.

Max, I really don't think Myk was trying to say that the whole move to threads is a regression. He's just referring to the UX, which if jittery is indeed a regression because 90% of the users probably won't notice the benefits of threading easily, but will definitely notice a jumpy match experience. They just want the game to work and look good. The improvements in codepaths will only be noticable to the few developers working with it. That said, I think we're all looking forward to this working as intended.

> What is your hurry? Take the time and do it right.
Now it would make sense, when there's some activity in this thread.

Some of the devs have just been a bit too busy in work and their personal lives to jump straight into the branch on a few day's notice.

[Max mtg]

Marc, the issue will get fixed for sure* in the two weeks that pass before users get this new code. Can this be a reason not to reintegrate the branch?

> Some of the devs have just been a bit too busy
Reintegration is reasonable in that case, so that a wider circle or people gets involved in testing.

____
* done in 20618

[Chris H.]

Thank you for all of the hard work Max.

I do plan on doing some testing as soon as I can set aside some time to do so. I have a few other pressing needs to address first.

We will need to wait on reintegrating your branch as I had a build failure this morning with the beta build and deploy.

[myk]

http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/package-summary.html#MemoryVisibility

Actions prior to "releasing" synchronizer methods such as Lock.unlock, Semaphore.release, and CountDownLatch.countDown happen-before actions subsequent to a successful "acquiring" method such as Lock.lock, Semaphore.acquire, Condition.await, and CountDownLatch.await on the same synchronizer object in another thread.

In other parts of code (where forge.FThreads.invokeInNewThread(Runnable, boolean) is used) the EDT caller returns soon after starting a new thread and must not change the game state.

Consider the following order of events for two threads, T1 and EDT:
EDT: writes state to a widget
T1: crosses cyclic barrier, widget state copied to T1
T1: writes state to widget, exits synchronized block
EDT: reads state from widget, does not see T1's changes

of course, this interaction could be true between any two threads, not necessarily involving the EDT.

I am still not entirely clear why we're using threads at all here, actually. I haven't been with the project long enough to know the background for this decision. Could someone explain it to me? What benefits are we expecting from using threads? How is it superior to enqueuing Runnables in the EDT for an asynchronous event-driven system?

[Max mtg]

myk, I don't need those synchronized blocks.
In your example EDT reads old state, in my code EDT waits on a CountDownLatch until T1 is finished. When it is, it calls CountDownLatch.countDown and performs no futher actions. So EDT ends up with a correct state.

But to be closer to my code, you need to switch T1 and EDT for the mentioned sample.


To recieve data from human player Forge used both synchronous methods (like GuiChoose) and asynchronous ones (setInput, give it a couple of Runnable at best - or choose a specialized input at worst). That lead to a number of continuation points hard to track which one follows another (see diff for SpellAbiltyRequirements to see that). A number of continuation points lead to duplicate code - there were different methods to cancel an ability with chosen targets or with ones yet to choose - and to more complicated routines.

See also this post viewtopic.php?p=113471#p113471 (5th case) for effects of the old-fashioned input model.
And read what I wrote a few posts above about the advantages of the new model.

[Max mtg]

sent to trunk at r20625

[Max mtg]

Thank you for all of the hard work Max.

I do plan on doing some testing as soon as I can set aside some time to do so. I have a few other pressing needs to address first.

We will need to wait on reintegrating your branch as I had a build failure this morning with the beta build and deploy.

Good to hear that you appreciate the job done.

I had no idea how to fix that failure and didn't interfer. As the beta is out, I have reintegrated soon - r20625.
I'll eagerly wait for _real_ bug reports. (Though UX are also fine)

[friarsol]

Crash for Winter Orb. I click my land, it untaps, my message window still says Waiting for Actions. I click on my other untapped land (cause yknow, what is it waiting for?) and then I got the following crash.

java.lang.RuntimeException: Trying to unlock input which is not locked! Do check when your threads terminate!
at forge.control.input.InputControl.unlock(InputControl.java:199)
at forge.FThreads$1.run(FThreads.java:102)
at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)

[myk]

myk, I don't need those synchronized blocks.
In your example EDT reads old state, in my code EDT waits on a CountDownLatch until T1 is finished. When it is, it calls CountDownLatch.countDown and performs no futher actions. So EDT ends up with a correct state.

What you are doing looks clever, but clever people have tried tricks like this before. Since I can't seem to convince you, perhaps this article can.

The event driven model is the one to use here. Not threads. This is the type of problem that gets reliably resolved by registering a Runnable callback with an input request and enqueueing it on the event loop. For example:

Code: Select all

final choices = ...;
SwingUtilities.invokeLater(new Runnable() {
  @Override public void run() {
    choice = getChoice(choices);
    doNextStep(choice);
  }});


Easy to write, easy to maintain, and easy to understand. No deadlocks or memory consistency issues. Guaranteed.