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

Fixed threading issue using a thread pool #37

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

marco-calautti
Copy link
Contributor

This addresses Issue #36
I tested it on Linux using GCC, and everything seems to be working fine. Needs testing on Windows with VC++.

@CookiePLMonster
Copy link
Collaborator

Looks good now, but needs squashing. I'll test on Windows later and report back.

@marco-calautti
Copy link
Contributor Author

Just squashed the pull request.

@CookiePLMonster
Copy link
Collaborator

I just realized - since m_checksumJobs is being waited for when the file write/view "finishes", thread pool could probably be a global variable instead of instantiating pools over and over for each view.

@marco-calautti
Copy link
Contributor Author

I just realized - since m_checksumJobs is being waited for when the file write/view "finishes", thread pool could probably be a global variable instead of instantiating pools over and over for each view.

Do you want me to do it?

@CookiePLMonster
Copy link
Collaborator

CookiePLMonster commented Oct 13, 2022

Do you want me to do it?

If you're up for it that'd be great. I haven't verified it locally but I see no reason why it wouldn't work.

@marco-calautti
Copy link
Contributor Author

I made the thread pool global a the level of the IsoWriter. Maybe having it in the main is not nice.

@CookiePLMonster
Copy link
Collaborator

Yep, that's a better idea than making it global-global indeed. Conceptually it also makes sense - if multiple ISO writers ever coexist in the same process, it would make sense for their workers not to fight for the thread pool.

Copy link
Collaborator

@CookiePLMonster CookiePLMonster left a comment

Choose a reason for hiding this comment

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

Checked on Windows, everything works as expected.

@marco-calautti
Copy link
Contributor Author

Great! I guess you are going to merge the pull request, then.

@CookiePLMonster
Copy link
Collaborator

I would if I could - I have no merge rights in this repository, only @Lameguy64 can merge.

@CookiePLMonster
Copy link
Collaborator

CookiePLMonster commented Oct 21, 2022

@marco-calautti For the time being I merged this PR to the develop branch of the fork we used to work on v2.00:
CookiePLMonster@3199947

I had to rebase your changes manually so git history might be imperfect - but you're welcome to open the same PR against the fork. In case you're interested, I created a new develop-old branch you could PR against, and then I'd erase my own merge and restore develop to clean state this way.

@marco-calautti
Copy link
Contributor Author

I see the develop-old is some commits behind the master of this repo. Is that fine?

@CookiePLMonster
Copy link
Collaborator

Yes, the master of this repo contains some changes we've not "downstreamed" yet.

@marco-calautti
Copy link
Contributor Author

@Lameguy64 any news in merging this?

@CookiePLMonster
Copy link
Collaborator

I don't think we're going to get an answer anytime soon, to be fair.

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.

None yet

3 participants