tonihele / OpenKeeper

Dungeon Keeper II remake
GNU General Public License v3.0
433 stars 41 forks source link

Asset conversion #369

Closed tonihele closed 4 years ago

tonihele commented 4 years ago

Yeah, nothing too fancy. Multithreaded asset conversion. For me it cut the conversion time form 7 minutes to 6 minutes. The truth is that we still just need to wait for the models & textures to be converted. Textures could maybe be threaded further as its own task. But I wouldn't bother.

The UI suffered a bit. I didn't want to invest to that too much. Since we might be moving to SWT etc. Maybe the code is now more clearer. Smaller classes and clearer responsibilities.

ufdada commented 4 years ago

Maybe the max thread count could use some further tweaking

Another read: https://jobs.zalando.com/en/tech/blog/how-to-set-an-ideal-thread-pool-size/?gh_src=4n3gxh1

tonihele commented 4 years ago

Don't know what to make of that link :) I can't even tell are our conversions more CPU or disk intensive. That is why the "general guess". For me it was 4 threads max and gave that mentioned speed boost yes. What are you suggesting?

ArchDemons commented 4 years ago

1 2 3

ArchDemons commented 4 years ago

Most of the time it takes to save models with JME3Exporter. Maybe multithreading is worth doing there?

It seems to me that tasks should be executed as before sequentially. Multithreading should be added only to model conversion

ufdada commented 4 years ago

What are you suggesting?

I think it won't make much of a difference if the model conversion is still single threaded :P. Like ArchDemons said, currently the biggest improvement could be made there.

tonihele commented 4 years ago

Yeah, the textures can be multithreaded with ease like I said. The models yes but not with ease. There are some considerations like materials and the ordering if I remember correctly so locking at least if not anything more complicated.

Just quickly looking at the profiles. It looks like textures are CPU bound and models maybe mode disk bound. The jME models are really huge compared to the KMFs. Something that I have always wondered a bit that we should just use the KMFs straight. But the reasoning behind the jME optimized models that you can open them at least somewhere, meaning jME SDK (for which, yes, we have also our own model viewer).

If I change it to sequential, it at least for me is slower. So it went slower with multithreading for you guys? :o

ArchDemons commented 4 years ago

So it went slower with multithreading for you guys? :o

6 minutes versus 7 is not such a big difference to optimize the code. If the problem of saving the model in the IO disk and its multithreading cannot be solved it, then you can even leave the old code. It is worth trying to save the model in the queue and empty it with threads. It may be possible to reduce the time to 3 minutes. Is it better than 6?

tonihele commented 4 years ago

It is still more than 10% improvement, right? :)

It is worth trying to save the model in the queue and empty it with threads.

Yeah maybe multithread it like this, then the original ordering and all would be preserved. It could work. But responsibility-wise this belongs to the task itself, the task can do this. We don't need to know it in our little naive converter manager.

ArchDemons commented 4 years ago
executorService.submit(() -> {
     convertModel(assetManager, entry, kmfFile, destination);
     // We can delete the file straight
     f.delete();
     i.incrementAndGet();
});

Executed 3:40. Without this code - 8:09

55% right ? ;)

ArchDemons commented 4 years ago

I liked the previous window design more. With two progress bar.

I think you should remove threading from execution tasks and add to models and textures conversions

tonihele commented 4 years ago

Fine, I'll see what I can do :)

I'll tweak it a bit, thanks for the comments. I also liked the old UI better. I had couple of designs for displaying the multithreaded progress (believe me, this was the last straw design... :) ). But they were just hard to do currently. But we have the MacOS issue, so that kinda forces us to do something and I'm currently leaning towards switching to SWT. But not now, not with this. So maybe revisit the designs then.

I had 2,5 options for the UI to be more specific:

ArchDemons commented 4 years ago

I'm currently leaning towards switching SWT

Why not javaFX ?

ufdada commented 4 years ago

As far as i know JavaFX is pretty much dead (at least for oracle). It's also not included anymore (at least since Java11)

tonihele commented 4 years ago

JavaFX was once my first choice but I kind of gave hope on it. Like @ufdada said, I also have these mixed feelings about it. It is removed from Java, which is both good and bad thing. Good that it is independent, it can version itself (although Java in general has picked up the release pace). And the bad is uncertainty. There is now some corporation behind it. I just don't know what to make of it, really mixed.

SWT on the other hand is mature and stable. Native look. 2Mb footprint. Our need is so small that we should just pick the easiest for the 2 tiny views we have. And this looks very simple. Can't think of any negative to say about it.

ArchDemons commented 4 years ago

As far as i know JavaFX is pretty much dead (at least for oracle). It's also not included anymore (at least since Java11)

With Oracle licensing policy, we will soon need to migrate to OpenJDK. The fact that JavaFX is not included in Java 11 is even good. Java EE moved to Jakarta EE under the wing of Eclipse. Mysql has an analogue - mariaDB. I am not against SWT. I even managed to work a little with it. But Eclipse technologies is driving me crazy.

tonihele commented 4 years ago

I'm already using AdoptOpenJDK (https://adoptopenjdk.net/) with OpenKeeper and all already. Works like a charm.

tonihele commented 4 years ago

Ok, how about this. We maximize the threading, this brought my machine to its knees, but I think it should. It is now 61% improvement. Stable one. At least on my computer it has now really stable running time of 2:35.

I multithreaded the textures so that each texture container (3 containers) are processed on own threads (if available) and since models can't be exactly messed with (unless locks are added to the converter itself, it is just not thread safe, it could even crash on concurrent access)... The exporting of each model is done in own threads (at least for me this is the heaviest part). This model I think has pretty good balance on I/O & CPU bound things, they are kept separate and that feels smart.

ArchDemons commented 4 years ago

Much better. Running time 5:10 to convert models and textures. Although it used to be 9-10 minutes.

ArchDemons commented 4 years ago

Excellent