Skip to content

Updates from turbo #197

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

Merged
merged 3 commits into from
Apr 3, 2025
Merged

Updates from turbo #197

merged 3 commits into from
Apr 3, 2025

Conversation

BlackIkeEagle
Copy link
Member

I think I have cherry-picked the relevant commits for there from https://github.com/animetosho/par2cmdline-turbo

@BlackIkeEagle
Copy link
Member Author

@animetosho feel free to indicate stuff I might have missed, not everything is trivial with the parpar changed mixed in there :)

@animetosho
Copy link
Contributor

not everything is trivial with the parpar changed mixed in there :)

Sorry about that - I wasn't intending to upstream most of this; I've generally tried to keep changes in par2cmdline-turbo minimal, outside of ParPar integration, so I'm not sure most of these are particularly relevant.

I removed OpenMP from par2cmdline-turbo because it simplifies building a static binary, and ParPar doesn't use it. So it was just easier to remove the few remaining uses of OpenMP.
par2cmdline, however, does depend on OpenMP for threading, so I don't think it's removal makes sense, unless you intend to completely remove it.

The ParPar backend computes progress a little differently, hence this change, but par2cmdline doesn't do this, so I don't think this is correct.
(par2cmdline's progress is based on each recovery block that's computed, whilst ParPar is based on how many input blocks have been consumed)

Defining NDEBUG does disable all assert checks. It might be faster, but there's less checks going on - don't know which way you want to go on this one, but thought I'd point it out regardless.

The libatomic stuff was because I was using C++11's atomic over OpenMP (due to OpenMP removal). If you're keeping OpenMP, that's not needed.

MAX_CHUNK_SIZE may require some experimentation with, but it might be useful to leave the capability in there regardless.

@BlackIkeEagle
Copy link
Member Author

not everything is trivial with the parpar changed mixed in there :)

Sorry about that - I wasn't intending to upstream most of this; I've generally tried to keep changes in par2cmdline-turbo minimal, outside of ParPar integration, so I'm not sure most of these are particularly relevant.

No worries, I was kindof getting it on par with par2cmdline-turbo.

I removed OpenMP from par2cmdline-turbo because it simplifies building a static binary, and ParPar doesn't use it. So it was just easier to remove the few remaining uses of OpenMP. par2cmdline, however, does depend on OpenMP for threading, so I don't think it's removal makes sense, unless you intend to completely remove it.

Ah ok I did not check if it was still threading so that will have to remain then, thanks for pointing it out

The ParPar backend computes progress a little differently, hence this change, but par2cmdline doesn't do this, so I don't think this is correct. (par2cmdline's progress is based on each recovery block that's computed, whilst ParPar is based on how many input blocks have been consumed)

Ok thanks, I blindly took it and none of my inital tests showed an issue.

Defining NDEBUG does disable all assert checks. It might be faster, but there's less checks going on - don't know which way you want to go on this one, but thought I'd point it out regardless.

Ok that one I'll drop then, for speed there is par2cmdline-turbo :)

The libatomic stuff was because I was using C++11's atomic over OpenMP (due to OpenMP removal). If you're keeping OpenMP, that's not needed.

Thanks for pointing that out

MAX_CHUNK_SIZE may require some experimentation with, but it might be useful to leave the capability in there regardless.

Thanks for all the feedback, you pointed out many things I missed.

And the changes will be much simpler as well, thanks.

Conflicts:
	src/libpar2internal.h
	src/par2creator.cpp
	src/par2repairer.cpp

+ add:

Fix compiler warning

Conflicts:
	src/par2repairer.cpp
@animetosho
Copy link
Contributor

animetosho commented Apr 3, 2025

The new set of changes look good to me 👍

Edit: maybe include this commit as well
This one may or may not be useful - I'll let you decide

@BlackIkeEagle
Copy link
Member Author

The new set of changes look good to me 👍

Edit: maybe include this commit as well

Sorry I squashed the introduction and the compiler fix in this one: 65b5126

This one may or may not be useful - I'll let you decide

I'll recheck it, I probably hastly discarded it

@BlackIkeEagle BlackIkeEagle merged commit 6898f30 into master Apr 3, 2025
@BlackIkeEagle BlackIkeEagle deleted the updates-from-turbo branch April 7, 2025 17:04
@BlackIkeEagle BlackIkeEagle mentioned this pull request Apr 27, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants