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

Reduce minimum memory amounts #1347

Merged
merged 1 commit into from Nov 23, 2023

Conversation

HyperSoop
Copy link
Contributor

See #1318

This PR reduces the how small heap can be to 8 MB.

Signed-off-by: HyperSoop <pasha.mirus@gmail.com>
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

Thanks!

@Scrumplex Scrumplex added enhancement New feature or request simple change labels Jul 11, 2023
@TheKodeToad
Copy link
Member

Why were 8MB and 4MB chosen?

@HyperSoop
Copy link
Contributor Author

Why were 8MB and 4MB chosen?

8MB is the least I could run rd-132211 (the first avaivable prototype of minecraft) with, and since I'm reducing 128 to 8 I might as well reduce 64 to 4.

@TheKodeToad
Copy link
Member

that seems quite arbitrary to me...

@HyperSoop
Copy link
Contributor Author

It would be neat to have you be able to set any value by typing it in but not have the arrow buttons get you below 128 but my programming skills extend only to changing values 🙃

@embeddedt
Copy link
Contributor

that seems quite arbitrary to me...

To be fair, so does the original limit of 128. 😉 Unless it was set this way to get the 128MB increments to happen by default, in which case it makes sense.

@getchoo
Copy link
Member

getchoo commented Jul 15, 2023

i'm a bit confused here as to what this is really for. 128mb is a pretty good minimum, and i'm sure any pc capable of playing minecraft in the first place has enough ram to handle it; and regardless of this being possible, lower heap sizes are known for causing fragmentation across the heap, lowering throughput, and potentially causing long gc times. i'd imagine this problem would only get worse when mod loaders are introduced as well

a better solution may be to follow the suggestion from the original issue and creating an option for lower heap sizes, as for a vast majority of cases, 128mb won't hurt and could avoid people having issues

@TheKodeToad
Copy link
Member

In my opinion, options for options isn't a good idea (and inconsistent with our previous changes such as removing the upper limit), and instead the indicator next to the box should change. HyperSoop seems to have slightly limited knowledge though xD.

@embeddedt
Copy link
Contributor

i'm a bit confused here as to what this is really for. 128mb is a pretty good minimum, and i'm sure any pc capable of playing minecraft in the first place has enough ram to handle it

Obviously 99% of players do not need to set the heap size lower than 128MB. However this facility is useful for developers like myself in order to test the absolute limit of our optimizations, and currently Prism does not provide a way to override the memory amount in the GUI through JVM arguments.

@getchoo
Copy link
Member

getchoo commented Jul 15, 2023

Obviously 99% of players do not need to set the heap size lower than 128MB. However this facility is useful for developers like myself in order to test the absolute limit of our optimizations, and currently Prism does not provide a way to override the memory amount in the GUI through JVM arguments.

i'm 100% not against it being possible, i just don't think it's something that should be shown as supported - since removing these checks would signify that this is a supported configuration. if this is mainly targeted for developers, it should be treated as such and not be something users need to worry about. having a separate option for this (preferably with a warning) to help users understand this is probably not something they want to mess would do exactly this in my mind.

...though in all honesty, i would still question the usefulness of testing/targeting 8mb heap sizes in the real world, regardless of how many optimization mods you throw at the game; however, i like to think one of the main goals of prism is configurability, so i am still in support of this being added as long as there warnings to help average users not misconfigure their jvm

@HyperSoop
Copy link
Contributor Author

HyperSoop commented Jul 16, 2023

128MB is already an absurdly small amount to expect the game to run on, that unless you optimize it to be able to, at which point there's no reason to not be able to go even lower. There's no reason anyone would set their memory to 8MB and come asking for support, just as there's no reason anyone'd do that with 128, and this so far has not been an issue.

With java settings, it's likely that if one does touch them they know enough about what they're doing for the settings to not need to be dummy-proofed at the expense of one's experience using them.

@Frontear
Copy link

an idea id like to toss in:

on first launch for a specific instance, if memory is set below a considered minimum (say 256M), a warning is printed in the logs (similar to the warning that shows for disabling the java compatibility check). Alternatively a popup could occur warning the user that the ram is set to a very low value and that it can cause issues. It can be dismissed, suppressed forever per instance, or accepted and prism closes mc and lets you change it.

its possible we could introduce a better memory model where by default, prism defines a larger upper limit and lower limit (lower limit 1G, upper limit 8-10G). It can then be disabled by a checkbox for “disable java memory model” or something in the same way the java compatibility check button exists. The main benefit of this would be to allow easier control for memory for inexperienced users who just want a simple, works method, but offers the option to allow fine tuning. The only thing it could go against is Prisms original design of being more technically approached and offering all options upfront.

final thing id add, i agree with getchoo that this should be supported but not encouraged. this means i think there should be some kind of warning against it, or an option to enable it preventing it from being accidentally accessed. this specific use case is so incredibly niche i doubt hardly even a handful of people would ever be interested in something like this, and if an unsuspecting user accidentally does this and has mc crash constantly on launch, theyll probably blame Prism and cluelessly complain about “why their mc lag”.

@getchoo
Copy link
Member

getchoo commented Jul 16, 2023

128MB is already an absurdly small amount to expect the game to run on, that unless you optimize it to be able to, at which point there's no reason to not be able to go even lower.

honestly this seems like a good argument for increasing the minimum or keeping it as is

There's no reason anyone would set their memory to 8MB and come asking for support, just as there's no reason anyone'd do that with 128

of course not, but it doesn't mean it won't happen. if a launcher lets you do something that will literally never work, that is very intuitive and could easily lead to some users thinking it's some kind of bug - and even if that doesn't happen, having that option is still pretty pointless to have enabled by default (emphasis here as again, i'm not saying this shouldn't be added, just be a separate option)

With java settings, it's likely that if one does touch them they know enough about what they're doing

not really. the whole point of things like the java auto detector and options such as memory is to make it accessible for those who don't know much about java settings otherwise. if we were talking about jvm args here, it'd make sense, but imo this doesn't apply to a simple to use gui option

at the expense of one's experience using them

i'm also not sure what the "expense" here is? it's not like there's a precedent for putting more advanced options behind a simple toggle. the java checker is a good example of this, and i wouldn't say that is any kind of "expense" for advanced users who choose to disable it

@TheKodeToad
Copy link
Member

Currently, there is no upper limit for memory. That's far more dangerous.

So whilst this could be revisited in future, it would be more consistent to remove the lower bound.

@getchoo
Copy link
Member

getchoo commented Jul 16, 2023

on first launch for a specific instance, if memory is set below a considered minimum (say 256M), a warning is printed in the logs (similar to the warning that shows for disabling the java compatibility check). Alternatively a popup could occur warning the user that the ram is set to a very low value and that it can cause issues. It can be dismissed, suppressed forever per instance, or accepted and prism closes mc and lets you change it.

i think something in the log might be a bit too easy to miss for something that could cause a lot of issues in-game, but the pop up isn't a bad idea. the warning we already have when you manually specify -Xm(x|s)<amount> too low could probably replaced with this and it might be a good compromise

its possible we could introduce a better memory model where by default, prism defines a larger upper limit and lower limit (lower limit 1G, upper limit 8-10G). It can then be disabled by a checkbox for “disable java memory model” or something in the same way the java compatibility check button exists. The main benefit of this would be to allow easier control for memory for inexperienced users who just want a simple, works method, but offers the option to allow fine tuning.

this might be a little much as prism's current defaults are ok (and upper limits are a bit more common to use with massive modpacks for example), but i do like the idea of an easy way to override whatever we end up doing

this specific use case is so incredibly niche i doubt hardly even a handful of people would ever be interested in something like this, and if an unsuspecting user accidentally does this and has mc crash constantly on launch, theyll probably blame Prism and cluelessly complain about “why their mc lag”.

well said :)

@embeddedt
Copy link
Contributor

Personally I think if the launcher will allow me to allocate 24GB when I have 16GB of actual memory (yes, I shot myself in the foot doing this once, and it caused a full system lockup) it is more consistent to remove the lower bound, and revisit memory management entirely in a later update, than to have differing behavior for the upper and lower bounds.

@TheKodeToad
Copy link
Member

This conversation is melting my brain. but yeah that's pretty much what i said and i therefore agree ^ 😆

@getchoo
Copy link
Member

getchoo commented Jul 16, 2023

Personally I think if the launcher will allow me to allocate 24GB when I have 16GB of actual memory (yes, I shot myself in the foot doing this once, and it caused a full system lockup) it is more consistent to remove the lower bound, and revisit memory management entirely in a later update, than to have differing behavior for the upper and lower bounds.

in a way i understand what you mean, but i don't think having one unintuitive feature (or rather a bug, given your experience) that lets you "shoot yourself in the foot" is a good reason to add the ability to do the same elsewhere, even until things are revisited. i think that ideally, prism (only by default) would protect you from doing either of things since it really doesn't make sense for a launcher to just allow you to have a borderline unusable game or lock up your entire system, as that opens open a lot of users to possible harm. these options should always have an opt-out switch though, for those who are interested in messing around

@Frontear
Copy link

shot in the dark here, but what if we make it cli-only, or something that can only be set through manipulating the config file of the instance directly. it adds an extra step so non-tech users wouldnt stumble on it while allowing the functionality. now sure, its a bit of a hassle and quite awkward, but its not a bad idea given actual power users usually mess with configs on a daily.

@Jaklans
Copy link

Jaklans commented Jul 26, 2023

shot in the dark here, but what if we make it cli-only

If the settings UI was reopened to the Java tab, would the value be overridden back to the UI minimum? One of the OP's posts implied that.

It seems consistent to me to let a use put too little memory just as much as too much memory. There could be an in gui warning if it is outside of expected values. Is there any basis for something like that in other areas of the UI?

image

@embeddedt
Copy link
Contributor

If the settings UI was reopened to the Java tab, would the value be overridden back to the UI minimum? One of the OP's posts implied that.

Yes. At the moment the workaround I was using is editing the instance config file on disk, which works until you re-open the settings UI, at which point the value is returned to the 128MB minimum.

@TheKodeToad TheKodeToad added the changelog:changed A PR that appears under "Changed" in the changelog label Jul 30, 2023
@Trial97 Trial97 linked an issue Aug 12, 2023 that may be closed by this pull request
1 task
@Trial97 Trial97 mentioned this pull request Sep 29, 2023
1 task
@Scrumplex Scrumplex added this to the 8.1 milestone Nov 23, 2023
@Scrumplex Scrumplex merged commit 364fb98 into PrismLauncher:develop Nov 23, 2023
15 checks passed
@Scrumplex Scrumplex added the manual backport PRs that have been backported manually label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:changed A PR that appears under "Changed" in the changelog enhancement New feature or request manual backport PRs that have been backported manually simple change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to choose very small heap sizes
8 participants