Im-dex / xray-162

XRay engine 1.6.2 (S.T.A.L.K.E.R: Call of Pripyat) evolution
Apache License 2.0
47 stars 12 forks source link

Replaced ttapi with thread pool #58

Closed jazzvaz closed 6 years ago

jazzvaz commented 6 years ago

Tested, working good. Felt less freezes; no more stupid ttapi initialization loops with long iterations, therefore, loading engine take less time. Hopefully, ttapi became less trivial and dumb.

jazzvaz commented 6 years ago

Visual Studio builds fine. Revert __stdcall?

Im-dex commented 6 years ago

The issue not with __stdcall, but 32 bit build is broken.

Xottab-DUTY commented 6 years ago

@jazzvaz I hope you wouldn't mind if I use your work in my repository =)

Xottab-DUTY commented 6 years ago
#ifdef _GPA_ENABLED
#include <tal.h>
#endif

Also, what is TAL? What's the purpose of it?

jazzvaz commented 6 years ago

Oh yes, this defined mess that i cleaned... @Xottab-DUTY of course I dont mind, now proper threading is possible :) TAL is unknown, deleted file...

Xottab-DUTY commented 6 years ago

@jazzvaz, hmm.... AppVeyor failed to build your previous commit. Now it tries to build your new commit, but I think it will fail too =(

Xottab-DUTY commented 6 years ago

Also, GPA is Intel Graphic Performance Analyzer by Intel

jazzvaz commented 6 years ago

What if we just remove Skin SSE for x86 with intrinsic functions?

Xottab-DUTY commented 6 years ago

@jazzvaz I think we should leave them.. Who knows.. But anyway, linker can't find not only *_SSE functions but _x86 too. This is strange..

jazzvaz commented 6 years ago

@Xottab-DUTY I still think it is because of the calling convention. Let see. I don't see any point now in GPA, as well as Skin_SSE. We focus more on x64 version and it is not being used there.

Im-dex commented 6 years ago

@jazzvaz You don't need to remove 32bit specific code because we keep it for debugging purpose, it is useful to compare the engine behavior when you have some kind of issues that could be 64bit related. About GPA: feel free to remove that.

jazzvaz commented 6 years ago

@Im-dex do you want me to revert Skin_SSE ? Do you think asm code is worth keeping?

Im-dex commented 6 years ago

@jazzvaz Oh, I see you have replaced it with *_x86 functions, I think it will be ok to keep that change.

jazzvaz commented 6 years ago

@Im-dex If this request is done, you can close two issues:

Replace xrCPU_Pipe TTAPI Rewrite xrCPU_pipe asm code

Im-dex commented 6 years ago

@jazzvaz Only the second one. The first task implies work-stealing executor integration.

jazzvaz commented 6 years ago

@Im-dex what do you mean exactly? Thread pool can do parallel execution like TBB or PPL, which was a part of Asynchronous tasks support.

Im-dex commented 6 years ago

@jazzvaz This thread pool implementation is pretty straightforward and you should manually balance the tasks across pool threads. Work-stealing based executors (or thread pools) such as PPL or TBB can balance the tasks across threads automatically.

jazzvaz commented 6 years ago

@Im-dex then i dont see the point of having separate cpi_pipe library, just move skin implementation to core, and add tbb or ppl as a separate library. This will be good?

jazzvaz commented 6 years ago

Or I can suggest to add "parallel for" implementation to the current thread pool.

Xottab-DUTY commented 6 years ago

I think that xrCPU_Pipe is too small to be a separate library =)

jazzvaz commented 6 years ago

Well, i think many libraries can be shrink to one, or even replace them with static linking.

Im-dex commented 6 years ago

I don't see anything wrong in storing cpu_pipe as a separate library. It's required for several modules only, whereas xrCore contains general purpose code wich is required for almost every module of the engine.

@jazzvaz

Or I can suggest to add "parallel for" implementation to the current thread pool.

I think that reinventing the wheel is not a good idea ;)

Well, i think many libraries can be shrink to one, or even replace them with static linking.

What profit will it bring?

Xottab-DUTY commented 6 years ago

Well, i think many libraries can be shrink to one, or even replace them with static linking.

What profit will it bring?

For example: One executable file that you just double click then ????? then PROFIT :D

ForserX commented 6 years ago

Позаимствую в OxyDev/xray-oxygen

Im-dex commented 6 years ago

@Xottab-DUTY

One executable file that you just double click then ????? then PROFIT :D

How it already works now, isn't it?

Xottab-DUTY commented 6 years ago

@Im-dex no, there is other dlls around in bin folder. I'm talking about static linking of all modules into one file. Like in Metro or Survarium and other games.

Im-dex commented 6 years ago

@Xottab-DUTY I still don't see any profit to have one fat binary.

jazzvaz commented 6 years ago

@Im-dex just making all xr components static will bring performance, less hell with dllimport/export, so portability will be improved. These components are only used in the engine and working together with it. Also, there is no api for other programs, therefore, I don't see any point of having xr dlls. One binary will be around 20-40mb, but I would not call it fat nowadays.

Im-dex commented 6 years ago

@jazzvaz

just making all xr components static will bring performance

I think it will be a very small performance improvement which does not affect the whole engine performance

less hell with dllimport/export

agree

These components are only used in the engine and working together with it

Don't forget about utils. Also, someday sdk could become a part of repo too

jazzvaz commented 6 years ago

@Im-dex forgot about sdk, agree with you

jazzvaz commented 6 years ago

Replaced xray second thread with stl thread

Im-dex commented 6 years ago

@jazzvaz previous issues still not resolved

jazzvaz commented 6 years ago

@Im-dex which issues?

jazzvaz commented 6 years ago

what are utils and why they are not being part of the vs project from cmake?

Xottab-DUTY commented 6 years ago

@jazzvaz, it's compilers for game levels and other utils.

ForserX commented 6 years ago

@jazzvaz, Im-Dex does not support them.

Im-dex commented 6 years ago

@ForserX you are wrong @jazzvaz you should pass -DBUILD_UTILS=YES to the cmake command line to enable utils projects generation

ForserX commented 6 years ago

@Im-dex, да ты сам тогда говорил, что пока не хочешь заниматься ими. :)

Im-dex commented 6 years ago

@ForserX Это не значит, что я их не поддерживаю ;)

ForserX commented 6 years ago

@Im-dex, могу тебе через PR как-нибудь для них фиксы отправить. Я их полностью подчинить умудрился.

jazzvaz commented 6 years ago

в utils одни повторения друг у друга

Xottab-DUTY commented 6 years ago

@jazzvaz, вообще много где повторения у друг друга...

Im-dex commented 6 years ago

@ForserX Отправляй :)

Не очень понял, какие повторения вы имеете ввиду?

jazzvaz commented 6 years ago

@Im-dex в разных utils повтоярется код и файлы, например:

cl_log.h xrThread.h

jazzvaz commented 6 years ago

@Im-dex check if the set_thread_name is fine, so I do it for utils

jazzvaz commented 6 years ago

Well, VS compiles fine

jazzvaz commented 6 years ago

@Im-dex any ideas why AppVeyor does not have SetThreadDescription?

Xottab-DUTY commented 6 years ago

AppVeyor builds for x86 platform.

@jazzvaz, What platform you are building? x64? Try to build on x86.

jazzvaz commented 6 years ago

@Im-dex for me both builds fine

Im-dex commented 6 years ago

@jazzvaz It seems that Appveyor host system is not a Win 10 and therefore cmake uses old sdk. I have set CMAKE_SYSTEM_VERSION to 10.0 in the build settings, think it should help.