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

Split up bootstrap tasks and submit individual ones to the executor. #29

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

RainWarrior
Copy link
Contributor

Same as #28, but without manual task splitting, since the executor is doing that.

@aikar
Copy link
Contributor

aikar commented Oct 16, 2018

This bombards the process with hundreds of threads (I think I saw it go over 1k). That is why I did what I did in my PR.

MC is using a Cached Thread Pool. Unless you change that on MC, this is not good.

Now, overall I do prefer the cleaner approach here, but really needs clarity that it needs to use a queued executor instead.

@ichttt
Copy link

ichttt commented Oct 17, 2018

Can confirm, 1.13.2 pre 1 creates over 1000 threads at startup (for reference: 1.13.1 needs about 50). This really lags out some systems completly.

aikar added a commit to aikar/DataFixerUpper that referenced this pull request Oct 17, 2018
As mentioned in Mojang#29, submitting all tasks to the executor
implies that the executor will have limited # of threads.

But if used with a cached thread pool, we will easily hit 1k threads+

This manually splits the work based on core count to a reasonable
split rate, and then if the submitter wants to limit threads
even more, it can pass an executor that limits thread count.
@LordRalex
Copy link

Just to mark this as a reference, this change introduced 2 bugs which will kill the game on old hardware (below minimum requirements, but generally dual cores with no SMT/HT available).

https://bugs.mojang.com/browse/MC-137353
https://bugs.mojang.com/browse/MC-138093

@winnipeg21
Copy link

When opening 1.13 my computer does 5fps causing other programs to also lag or crash.

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.

7 participants