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

Increase streaming memory setting limit #2264

Merged
merged 3 commits into from
Aug 4, 2021
Merged

Conversation

Pirulax
Copy link
Contributor

@Pirulax Pirulax commented Jun 17, 2021

New limits:

// Streaming memory range based on system installed memory:
//
//     System RAM MB     min     max
//           512     =   64      96
//          1024    =   96     128
//          2048    =  128     256
//          4096    =  128     384
//          6146    =  128     512

Untested, but probably works as expected.` Tested

Add new steps (System RAM => min/max streaming memory limit):
- 4096 => 128/512
- 6144 => 128/768
- 8192 => 128/1024 - Questionable if this is necessary, 1 GiB seems a like a lot...
@Pirulax Pirulax marked this pull request as draft June 17, 2021 22:23
@Dutchman101
Copy link
Member

Much more about this was said in the MTA development discord (e.g #streaming-memory channel), including that by now Pirulax tested it and it seemed to work properly.

I also don't see a problem with this, as long the existing older code that this "dynamically decide how much VRAM is a good idea" will work properly with these new limits (as in: does it not need a refactor or further modernisation rather than simply adding higher limits) - you'll probably need a final debug build test where you load heavy mods and stretch streaming memory usage all the way up to its new limits.

The main concern for previous calls to raise streaming memory limits, is for users with on-board GPU (where as the demand by a game grows it will draw more and more shared memory from available system RAM, leading to a risk for out-of-mem crashes).

This code looks like the thing I proposed elsewhere last week, to get around that concern: to let MTA dynamically assign a safe amount of streaming memory, based on the available amount of system RAM (to accept the chance player has on-board GPU). Against my expectations it already existed in the shape of this old code, which by the way got added in 810bf9c and when it got introduced initially it supported 512MB streaming memory for users with 2GB RAM, and that later got nerfed down to 256MB streaming memory.

I guess the reason for that was reconsideration (just being more careful), but also obviously back then (in 2012) the average PC had different specs and therefore I think that the updated table (per the changes in this PR), which currently looks like this:

// Streaming memory range based on system installed memory:
//
//     System RAM MB     min     max
//           512     =   64      96
//          1024     =   96     128
//          2048     =  128     256

And in its updated form:

// Streaming memory range based on system installed memory:
//
//     System RAM MB     min     max
//           512     =   64      96
//          1024    =   96     128
//          2048    =  128     256
//          4096    =  128     512
//          6146    =  128     768
//          8192    =  128     1024

Looks way more appropiate for this age, today's PC specs.

@Pirulax you just forgot to update the table as well (here: CCore.cpp @ 1912 along with the changes this PR makes

@Pirulax
Copy link
Contributor Author

Pirulax commented Jun 18, 2021

Yeah, the table, thanks!

The 1 GiB @ 8 GiB RAM might be too much, because heres a thing: MTA likes keeping models in memory (see CModelCacheManager).
Also, GTA doesn't seem to unstream models unless the memory limit is reached, which basically means that.. The memory available is likely to be used all up, which by itself woudln't be an issue if GTA was x64, but it isn't which means we have a finite amount of address space.

As of now these limits are prefectly fine (for the vanilla game), and an increase might lead to crashes (as people are likely to max it out, because they think, since they have lots of RAM, they can max it out, but thats not the full picture) as it might push over the gap on some servers.

So, I propse to lower the maximums a little:

// Streaming memory range based on system installed memory:
//
//     System RAM MB     min     max
//          512     =   64      96
//          1024    =   96     128
//          2048    =  128     256
//          4096    =  128     384
//          6146    =  128     512

I created this PR with #1677 in mind, as thats where the higher stream memory is basically a must, but for that we should add a function (as discussed earlier), because allowing people to max this setting out might cause more issues than good.

@Dutchman101
Copy link
Member

Dutchman101 commented Jun 18, 2021

The 1 GiB @ 8 GiB RAM might be too much

Maybe you should reduce the new maximum to 512MB or 768MB streaming memory. I would go for the latter.

Or maybe first go for 512MB, see the metrics & issue reports and if everything turns out fine (no increase in crashes) you can finally raise it to 768MB and see again.

Well, i just saw your edit:

So, I propose to lower the maximums a little:

// Streaming memory range based on system installed memory:
//
//     System RAM MB     min     max
//           512     =   64      96
//          1024    =   96     128
//          2048    =  128     256
//          4096    =  128     384
//          6146    =  128     512

Looks good as well i guess

@PlatinMTA
Copy link
Contributor

Could you guys add another metric? 16GB RAM -> up to 1GB on streaming memory

This shouldn't affect 16GB users and would be appreciated.

BTW thank you for this pull!

@Pirulax
Copy link
Contributor Author

Pirulax commented Jun 18, 2021

Heck, I could add that even with 8 GB, but there are diminishing returns after 800 megs. 1024 streaming memory is 4 times the amount vanilla GTA uses.

If GTA was 64 bits I'd be glad to increase the limit to a gig, but wasting 1 / 4 of total available memory to streaming isn't worth it.

@Dutchman101
Copy link
Member

We should keep adding support for 768MB streaming memory in mind, if it later turns out no one had issues with the maximum of 512MB that this PR (after last changes) raises it to.

@Pirulax
Copy link
Contributor Author

Pirulax commented Jun 18, 2021

I dont see how the vanilla game would benefit from 768, but sure, worst case scenario we revert the pr.

@StrixG StrixG added the enhancement New feature or request label Jun 19, 2021
@Pirulax Pirulax marked this pull request as ready for review August 2, 2021 22:53
@Pirulax
Copy link
Contributor Author

Pirulax commented Aug 2, 2021

I think we should just go ahead and merge it.
Just remember that users will have to manually increase the slider, as the setting stores absolute values.. So nightlies wont be too effective in catching stuff possible issues..

Copy link
Contributor

@patrikjuvonen patrikjuvonen left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@patrikjuvonen patrikjuvonen added this to the Next Release (1.5.9) milestone Aug 4, 2021
@patrikjuvonen patrikjuvonen merged commit ab2d769 into master Aug 4, 2021
@patrikjuvonen patrikjuvonen deleted the Pirulax-patch-1 branch August 4, 2021 15:51
@Dutchman101
Copy link
Member

Just remember that users will have to manually increase the slider, as the setting stores absolute values.. So nightlies wont be too effective in catching stuff possible issues..

Do you mean people with existing installations (and thus config) being bound to 128MB unless they change it after the update? In that case, i guess it will use the dynamically calculated amount of streaming memory for new installations (following the table of new limits)?

If it's like that, it can be worth to clear the values with the update, for all users, once

Pirulax added a commit to Pirulax/mtasa-blue that referenced this pull request Aug 28, 2021
patrikjuvonen pushed a commit that referenced this pull request Sep 5, 2021
@Xenius97 Xenius97 mentioned this pull request Jun 22, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants