Cleanup and hoping to increase performance

[Sloth]

You might have noticed that i commited some risky code changes in the last couple of days. The reason is that forge has become rather slow on one of my two computers (and some users have reported forge to behave more sluggishly) and i want to see which part is responsible for this.

Here are my results so far:
1. The AI is not responsible for big slowdowns (i can completey shut off playSpellAbilities and it won't change much).

2. Reducing heap size has lots of potential and could speed up loading times and maybe slowdowns on some systems. The biggest chunk by far is the card class, because two copies of each card are loaded on start up (one "small" object for the deck editor and a "full" object for the game). Just changing a few ArrayLists to start as null pointers instead of empty lists reduced the heap space needed by about 2%.

3. In the end, i'm confident that the GUI is responsible for the biggest slowdowns. Removing a few of the "updateObservers" calls made forge a little more fluid already (on my slow computer at least) and i notice a little pause before the start of every animation (cards moving into hand for example).

I'm just a hobby programmer so maybe i'm missing the most important things here. Is there anything you can add to this topic that might help?

[Chris H.]

Forge's findbugs report has a few performance type errors and fixing them may provide some relief?

[friarsol]

I think you are definitely right the UI is part of the main causes to the slowdowns. But just playing a quick quest game it looks like the power toughness values take a few phases before they update. The AI cast a Murkfiend Liege and three phases later all his creatures changed P/T. And when I destroyed the Liege it took a few more phases before they reset.

[Max mtg]

Hey, Sloth
I have removed some shit left by Braids (use of Generators to iterate over filesystem), some exessive arraylists creation and turned card parsing code into a little state machine. Got -25% heap usage.

The other big piece of improvements is really about using the heavy Card class only when neccesary - never use it in deck editors or other deck generator. If you stop Forge at the menu screen there are already 22 thousand instances of forge.Card ... while there should be zero.

[Sloth]

Hey, Sloth
I have removed some shit left by Braids (use of Generators to iterate over filesystem), some exessive arraylists creation and turned card parsing code into a little state machine. Got -25% heap usage.

The other big piece of improvements is really about using the heavy Card class only when neccesary - never use it in deck editors or other deck generator. If you stop Forge at the menu screen there are already 22 thousand instances of forge.Card ... while there should be zero.

It's good to have some help Max. With r16353 i get two compile errors though: It seems that CardFactory was used as iterable in two places.

I've also noticed that random deck generation is now buggy (colors don't match with basic lands anymore).

EDIT: Most of the time generate random deck will fail.

[Chris H.]

With r16353 i get two compile errors though: It seems that CardFactory was used as iterable in two places.

 
Can only iterate over an array or an instance of java.lang.Iterable
GuiDownloadPicturesLQ.java
/ForgeSVN/src/main/java/forge/gui/download
line 71

Can only iterate over an array or an instance of java.lang.Iterable
GuiMigrateLocalMWSSetPicturesHQ.java
/ForgeSVN/src/main/java/forge/gui
line 496

[Sloth]

The mana cost overlays also behave flakey now.

[friarsol]

When we're preloading card images, could we keep track of the images that weren't found instead of needing to go back through CardFactory in the Download Pictures functions?

[Max mtg]

That's strange... looks like collection.tounmodifiablesomething does not copy the collection. Ok, i'll find a method - maybe replacing the arraylist with pure array will improve the manaparser perf. Thanks for the bug report.


Sol, I am not sure. The actual system has an advantage that it can catch graphics as you copy it into the right folder. You'll have to reload the app to have that file list built for actual fs state, not the one by the moment application has started.

The real challenge is to have cardfactory load cards by request. It should have card-printed always ready, and Card instances will all the spellabilities and friggers to become lazily loaded. I am sure that will release another hundred of megabytes of heap

[friarsol]

Ok, it looks like the mana pool count isn't updating until you finish paying the whole cost right now, which is terrible UI.

[Sloth]

Ok, it looks like the mana pool count isn't updating until you finish paying the whole cost right now, which is terrible UI.

This should be working now. I still removed the UpdateObserver call from clearManaPool, because that only happens when priority shifts. Thanks for checking sol.

[Max mtg]

I have just removed a "creating card objects step" from loading sequence. The game still compiles and works! Was that step needed at all?

[friarsol]

I have just removed a "creating card objects step" from loading sequence. The game still compiles and works! Was that step needed at all?

I believe we had a discussion at one point regarding this (before CardPrinted objects existed) and determined it was better to load card objects in the startup script than loading faster and then needing to wait again for Card objects to be created for opening the deck editor or starting a game. It does make more sense only to load Card Objects before a match (now that CardPrinted objects exist), and we'll only need to have a few hundred full Card objects in memory at a time, instead of 11k all of the time.

[Max mtg]

I believe we had a discussion at one point regarding this (before CardPrinted objects existed) and determined it was better to load card objects in the startup script than loading faster and then needing to wait again for Card objects to be created for opening the deck editor or starting a game. It does make more sense only to load Card Objects before a match (now that CardPrinted objects exist), and we'll only need to have a few hundred full Card objects in memory at a time, instead of 11k all of the time.

Look, when "preloading card images" runs, all the Card objects are already created once. So the second run was absolutely useless - it collected all copies into a single list for an unknown by now purpose.

I am already working on code that would create Card objects from CardPrinted objects. And, yes, getCard(String, ...) will be removed from cardfactory interface

[Max mtg]

ok, let's see: r16375 has implemented forge.Card on-demand creation.

Post bug reports here

Upd: a picture to compare.
Upd2: I've only optimized the startup to reduce heap size. The other source of complaints (ai and gui slowdowns) are yet to be fixed