Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added parallel mode to utilise power of all CPUs #67

Merged
merged 21 commits into from
Mar 15, 2021

Conversation

doterax
Copy link
Contributor

@doterax doterax commented Mar 10, 2021

Taskflow added from sources to make AppVeyor happy
Changed c++14 to c++17 to support taskflow
Added option -p --parallel to enable parallel mode
Logs changed to prevent mess in it

@doterax
Copy link
Contributor Author

doterax commented Mar 10, 2021

Need help with building these configurations: Clang-Release; Platform: x86, Clang-Release; Platform: x64
AppVeyor build failed with error: cannot use 'throw' with exceptions disabled

Copy link
Owner

@JayXon JayXon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request! Overall looks pretty good, a couple things:

taskflow

Could you move all the taskflow files into the lib directory? That's where all the third party libraries live.
Also since we are checking in code into this repo, we should remove files not needed, for example we probably don't need any of those cuda or tensor files...

AppVeyor failures

Clang-Release; Platform: x86 you missed one of the <ExceptionHandling>false</ExceptionHandling> lines
Clang-Release; Platform: x64 this seems to be something else, I'm not sure why it's complaining, but doesn't look like your fault, I'll see if I can fix this. fixed in 7075d27

Travis CI failures

I'm not sure why it doesn't show up in this PR, but it's building this and all failed: https://travis-ci.org/github/JayXon/Leanify/builds/762323769

Logs

I see that you've reordered one of the logs, but that doesn't address any verbose logs, (try -c -v), do you have any plans for those?

main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
@doterax
Copy link
Contributor Author

doterax commented Mar 11, 2021

Thank you for review!

I processed all your requests. Except removing not needed files from taskflow. That because I have some plans for them.

Looks like problems with linking in Clang configurations. I will check them tomorrow on a different machine. Or if you have suggestions don't hesitate to contact me.

Cheers :)

Copy link
Owner

@JayXon JayXon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I processed all your requests. Except removing not needed files from taskflow. That because I have some plans for them.

Interesting, do you mind sharing the plans?

Looks like problems with linking in Clang configurations. I will check them tomorrow on a different machine. Or if you have suggestions don't hesitate to contact me.

Probably missing pthread?

main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
@doterax
Copy link
Contributor Author

doterax commented Mar 12, 2021

Interesting, do you mind sharing the plans?

Yes, sorry.

My future plans is:
- change the way how comand line arguments processed
- add more command line options for zopflipng (for example diffirentiate number of iterations for small and big files for zopfli)
- try playing with guetzli-cuda-opencl for JPEG (https://github.com/ianhuang-777/guetzli-cuda-opencl/tree/opencl)

@doterax
Copy link
Contributor Author

doterax commented Mar 12, 2021

Travis CI build failed due to not full support of C++17.

https://stackoverflow.com/questions/40177006/c-variant-no-such-file-or-directory

@JayXon
Copy link
Owner

JayXon commented Mar 13, 2021

I don't think we can use guetzli in leanify, because leanify is a lossless tool but guetzli is lossy.

Travis CI build failed due to not full support of C++17.

Sorry about this, I've already switched from travis ci to github actions, it should show up if you add a new commit.

@doterax
Copy link
Contributor Author

doterax commented Mar 14, 2021

Could you address this? I don't see any new commits

Yes, sorry I set up system to reproduce Clang linking issues and didn't commit before fix. And now AppVeyor works.

I had to disable optimisation for Clang, and this helped.
Someone also faced same issue taskflow/taskflow#300

I don't think we can use guetzli in leanify, because leanify is a lossless tool but guetzli is lossy.

Ok, I understand. I removed all unused taksflow files.

I've already switched from travis ci to github actions, it should show up if you add a new commit.

gcc build on ubuntu-16.04 still not fully support C++17

Rest configurations on ubuntu are failed, and for now I have no idea how to fix them. Because problem looks same as in Clang, I tried to remove optimisation from makefile, but this is not worked.

@doterax doterax changed the title Added concurrent mode to utilize power of all CPUs Added parallel mode to utilise power of all CPUs Mar 14, 2021
Copy link
Owner

@JayXon JayXon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to disable optimisation for Clang, and this helped.
Someone also faced same issue taskflow/taskflow#300

Hmm I think we should figure out why it doesn't work with optimizations, right now this is a regression for using leanify on single file, at least I want to know exactly which optimization is causing this issue, do we really need to disable all the optimizations?

gcc build on ubuntu-16.04 still not fully support C++17

Excluded that from the matrix

Rest configurations on ubuntu are failed, and for now I have no idea how to fix them. Because problem looks same as in Clang, I tried to remove optimisation from makefile, but this is not worked.

Try adding -pthread to the makefile?

main.cpp Show resolved Hide resolved
@doterax
Copy link
Contributor Author

doterax commented Mar 15, 2021

Try adding -pthread to the makefile?

It works. Thank you.

Do we really need to disable all the optimizations?

I'm trying to figure out why it helps and how to preserve all optimizations.

Makefile Outdated Show resolved Hide resolved
@doterax
Copy link
Contributor Author

doterax commented Mar 15, 2021

I'm trying to figure out why it helps and how to preserve all optimizations.

I'm working on it. Looks like rare LLVM bug. Have plans to rewrite TLS part of taskflow an enable optimisation for Clang. Also somehow SEH on Windows also involved.

@doterax
Copy link
Contributor Author

doterax commented Mar 15, 2021

Looks like everything is green now :)

@JayXon JayXon merged commit 3e0d2d7 into JayXon:master Mar 15, 2021
@JayXon
Copy link
Owner

JayXon commented Mar 15, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants