Configurable Gradual Chunkbuilder Thread Creation #39
Conversation
| // Yikes. | ||
| this.initialTime = currentTimeMillis(); | ||
| this.quickThreadCreationInterval = MathHelper.clamp(SodiumClientMod.options().advanced.quickThreadCreationInterval, 1, 2000); | ||
| this.slowThreadCreationInterval = MathHelper.clamp(SodiumClientMod.options().advanced.slowThreadCreationInterval, 1, 60000); |
There was a problem hiding this comment.
speedrunapi will enforce these bounds by default, so the clamp here is redundant
| * Returns the "optimal" number of threads to be used for chunk build tasks. This will always return at least one | ||
| * thread. | ||
| */ | ||
| private static int getOptimalThreadCount() { |
There was a problem hiding this comment.
imo it's a little confusing to reuse the "getOptimalThreadCount" method name for a different purpose here, maybe it would be better to just call it "getSuggestedTargetThreadCount" or similar to keep the naming system consistent between methods and fields?
There was a problem hiding this comment.
That's fair, I'll rename it to getDefaultTargetThreads to align
|
Note - SeedQueue PR is created but as mentioned by Contaria, we should probably also port this to MacSodium (otherwise seedqueue will break on macs, which is - probably - suboptimal) |
|
that, and I'd like to make a couple changes to this pr and that pr to re-add compatibility to old sodium versions and not make it too brittle |
Sounds good, however you want to go about it. I wasn't sure how worthwhile some of the changes were because most of it seemed to be in profiling code with the preceding "this will be removed" comment. I'm not sure if that's still the plan (& if we could just ... remove the profiling code now? ^^) or if we do need to retain those methods. |
|
contaria said he wanted to keep those, that comment is inaccurate |
|
|
Sounds good, let me know if you want me to make the changes, or if it's easier to just push directly to the branch/make a PR. |
DesktopFolder
force-pushed
the
jojoe-based-gradual-thread-creation
branch
from
36f0211 to
24a26a2
Compare
contariaa
left a comment
contariaa
left a comment
There was a problem hiding this comment.
Is the plan to keep these options around forever or to have them there temporarily for widespread testing and finding optimal values?
I'm always weary of giving users too much control over threading related stuff because they almost never know what theyre doing but threading is quite the magic word when it comes to performance.
src/main/java/me/jellysquid/mods/sodium/client/render/chunk/compile/ChunkBuilder.java
Outdated
Show resolved
Hide resolved
SeedQueues sodium profiling mixins are disabled on sodiummac already, so the maintenance burden shouldnt be too bad hopefully, see here. |
I thought about this a lot when initially making these changes - ultimately I think the issue is that, because these settings all do affect user performance quite a bit (observably), and they all have different use cases to be set to different values, there isn't really a way to get around this level of configurability for the end-user. However, I also kept in mind the theoretical that one day, there might be a port of a newer Sodium version, and we'd be able to leave most/all of this behind. |
4 participants
See #38 and #36 for justification/prior art.
Adds a lot of configuration options. Probably requires user testing to ensure this works.
This breaks SeedQueue compatibility. (Likely minor) changes would be required to fix mixin compatibility & ensure no regression.