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

Fix Texture load loop on memory warnings #11

Closed
wants to merge 1 commit into from

Conversation

rampr
Copy link

@rampr rampr commented Aug 15, 2015

Solves 3 cases

  1. when memory_warning happens very early, the value highest sometimes is 0.
    This leads to a load loop because we are trying to ratchet down or increase
    memory against 0, which would stay 0, because MEMORY_DROP_RATE and MEMORY_GAIN_RATE
    are being multiplied against 0. So we set highest as 50MB (Please suggest a different value).
  2. We are increasing the max_texture bytes on two conditions,
    a. when it is overLimit
    b. when highest > max_texture_bytes (this is already been doing right now)
  3. overLimit calculation had a bug in texture_manager_clear_textures() function.
    manager->approx_bytes_to_load will reduce when textures are cleared, but that was
    not taken into account in the overLimit calculation.

Solves 3 cases

1. when memory_warning happens very early, the value highest sometimes is 0.
This leads to a load loop because we are trying to ratchet down or increase
memory against 0, which would stay 0, because MEMORY_DROP_RATE and MEMORY_GAIN_RATE
are being multiplied against 0. So we set highest as 50MB (Please suggest a different value).

2. We are increasing the max_texture bytes on two conditions,
    a. when it is overLimit
    b. when highest > max_texture_bytes (this is already been doing right now)

3. overLimit calculation had a bug in texture_manager_clear_textures() function.
manager->approx_bytes_to_load will reduce when textures are cleared, but that was
not taken into account in the overLimit calculation.
@rampr
Copy link
Author

rampr commented Aug 15, 2015

This fixes 1 issue of play-co/devkit#240

@rogueSkib
Copy link
Contributor

Awesome, thanks for the PR! I'm going to test this out on our games

@rampr
Copy link
Author

rampr commented Aug 25, 2015

Hi Jimmy, Did you get a chance to test this out ?

@rogueSkib
Copy link
Contributor

I haven't yet but it's on my short list! It looks good to me, we just have
to be thorough in testing it, which is why I haven't had time yet.
Definitely haven't forgotten about it, sorry for the delay!

On Tue, Aug 25, 2015 at 8:08 AM, ram [email protected] wrote:

Hi Jimmy, Did you get a chance to test this out ?


Reply to this email directly or view it on GitHub
#11 (comment)
.

Jimmy Griffith
704.309.2473
Studio Head @ Weeby.co
http://www.weeby.co

@rogueSkib
Copy link
Contributor

@rampr I've tested the changes on a range of phones, and I found some issues on the low-end (Samsung Galaxy Y).

I've started a new pull request so I could more easily edit and merge the code as I test: #12

Per your fix for 1), that checks out and I'm starting the value at 1 MB.
For 2), can you elaborate on why this change was necessary? Did you find your app needed to more aggressively scale the memory limit?
3) had to be removed. This change causes a new kind of texture loop, especially when using offscreen canvases. The change effectively makes texture_manager_clear_textures clear fewer textures, which keeps the app closer to the estimated memory limit. In cases where the memory limit is accurate, we are highly likely to fail on loading new textures. I found that this made our offscreen canvases flicker as the memory limit hung very closely to the actual device memory limit, instead of throwing out enough textures to make room.

@rampr
Copy link
Author

rampr commented Sep 1, 2015

@rogueSkib, 2) was what actually fixed the flickering issue for us

Once max_texture_bytes was increased to a safe value more than(epoch highest), what we saw was that texture_manager_clear_textures was clearing textures because overLimit was reached, but since these are to be shown on screen, they were again loaded and then again overLImit reached and then cleared and the loop continued.

What I felt was a mismatch because overLimit is being used for clearing textures, but not being used when increasing the memory limit.

I have logs for the above.
Before the fix -> https://s3.amazonaws.com/emacsian/flickering.log
After the fix -> https://s3.amazonaws.com/emacsian/flickering_fix_overlimit.log

We also tinkered the value of MEMORY_GAIN_RATE and that did stop the flickering, but felt that may not work for all apps/devices.

About 3), I think it is a bug, because overLimit depends on adjusted_max_texture_bytes and that is based on bytes_to_load, but this computation should happen after each texture clear, but is only being done before iterating through all the textures.
I understand that this will just clear enough textures and the memory limit might be close to the estimated memory limit. It wasn't affecting us, I happened to come across when debugging the flickering issue. If you don't want this change, maybe we should have a comment then this is intentional.

@rogueSkib
Copy link
Contributor

@rampr Thanks a ton for your help resolving this issue. I've updated my pull request with the changes from 2), can you confirm that branch resolves your issue? I'll merge it and close this afterwards. Thanks again!

#12

@rampr
Copy link
Author

rampr commented Sep 3, 2015

@rogueSkib, :) I'll test out the branch today.
About 3, shouldn't we have a note in the code that it is intentional ?

@rogueSkib
Copy link
Contributor

I think this might be a misunderstanding. The comments are as follows:

    /**
     * Rules for Clearing Textures:
     * 1. the texture must be loaded to be cleared, period
     * 2. throw out all textures if clear_all is true, but respect rule 1
     * 3. throw out failed textures, forcing them to reload if needed
     * 4. throw out least-recently-used textures if we exceed our estimated memory limit
     */

As you noticed, adjusted_max_texture_bytes depends on approx_bytes_to_load. This value can only be modified during the clear texture iteration if the texture being cleared is not loaded, (line 519-521). The first rule of clearing textures in the comments is that a texture must be loaded to be cleared, as seen in if (tex->loaded && (clear_all || tex->failed || overLimit)) { (line 405). Therefore the value of adjusted_max_texture_bytes never changes during this iteration. The value texture_bytes_used does get updated though, which is why overLimit is recalculated on each iteration.

@rampr
Copy link
Author

rampr commented Sep 3, 2015

In our case, when flickering was happening, there were textures on screen and hence loaded, since overLimit was reached, some were cleared and again since they had to be be shown on screen, they had to be loaded leading to flickering.

In this particular case, since texture was cleared, approx_bytes_to_load would get changed and hence the overLimit calculation would be wrong.

Same output from logs, textures getting freed were present on screen since it was overLimit and again getting loaded because they had to be shown on screen:

2015-08-12 11:22:17.145 TeaLeafIOS[251:13839] {tex} Before: loading.png canvas=0 access=1439358732 tex-epoch: 0 frame-epoch: 1
2015-08-12 11:22:17.145 TeaLeafIOS[251:13839] {tex} Before: spritesheets/resources-images-map_screen0.png canvas=0 access=1439358735 tex-epoch: 0 frame-epoch: 1
2015-08-12 11:22:17.145 TeaLeafIOS[251:13839] {tex} Before: spritesheets/resources-images-menu_screen0.png canvas=0 access=1439358736 tex-epoch: 1 frame-epoch: 1
2015-08-12 11:22:17.145 TeaLeafIOS[251:13839] {tex} Before: spritesheets/resources-images-menu_screen1.png canvas=0 access=1439358736 tex-epoch: 1 frame-epoch: 1
2015-08-12 11:22:17.145 TeaLeafIOS[251:13839] {tex} Before: spritesheets/resources-images-tutorial0.png canvas=0 access=1439358735 tex-epoch: 0 frame-epoch: 1
2015-08-12 11:22:17.145 TeaLeafIOS[251:13839] {tex} Before: @TEXTAsap-Regular|36|175|47|25|255|315|0|0|Play canvas=0 access=1439358736 tex-epoch: 1 frame-epoch: 1
2015-08-12 11:22:17.146 TeaLeafIOS[251:13839] {tex} Before: @TEXTAsap-Regular|36|175|47|25|255|315|0|0|How to Play canvas=0 access=1439358736 tex-epoch: 1 frame-epoch: 1
2015-08-12 11:22:17.147 TeaLeafIOS[251:13839] {tex} Before: @TEXTAsap-Regular|35|255|255|255|255|274|0|0|Connect canvas=0 access=1439358736 tex-epoch: 1 frame-epoch: 1
2015-08-12 11:22:17.147 TeaLeafIOS[251:13839] overLimit: 1
2015-08-12 11:22:17.147 TeaLeafIOS[251:13839] overLimit: 1
2015-08-12 11:22:17.147 TeaLeafIOS[251:13839] overLimit: 1
2015-08-12 11:22:17.148 TeaLeafIOS[251:13839] overLimit: 1
2015-08-12 11:22:17.148 TeaLeafIOS[251:13839] overLimit: 1
2015-08-12 11:22:17.148 TeaLeafIOS[251:13839] overLimit: 1
2015-08-12 11:22:17.148 TeaLeafIOS[251:13839] {tex} Texture freed: @TEXTAsap-Regular|36|175|47|25|255|315|0|0|Play!  COUNT=7, USED=131072
2015-08-12 11:22:17.162 TeaLeafIOS[251:13839] overLimit: 1
2015-08-12 11:22:17.162 TeaLeafIOS[251:13839] {tex} Texture freed: @TEXTAsap-Regular|36|175|47|25|255|315|0|0|How to Play!  COUNT=6, USED=65536
2015-08-12 11:22:17.163 TeaLeafIOS[251:13839] overLimit: 0
2015-08-12 11:22:17.163 TeaLeafIOS[251:13839] {tex} Unloaded 2 stale textures. Now: Texture count = 6. Bytes used = 163840 -> 65536 / 65536
2015-08-12 11:22:17.163 TeaLeafIOS[251:13839] {tex} {tex} After: loading.png canvas=0 access=1439358732
2015-08-12 11:22:17.163 TeaLeafIOS[251:13839] {tex} {tex} After: spritesheets/resources-images-map_screen0.png canvas=0 access=1439358735
2015-08-12 11:22:17.164 TeaLeafIOS[251:13839] {tex} {tex} After: spritesheets/resources-images-tutorial0.png canvas=0 access=1439358735
2015-08-12 11:22:17.164 TeaLeafIOS[251:13839] {tex} {tex} After: spritesheets/resources-images-menu_screen0.png canvas=0 access=1439358736
2015-08-12 11:22:17.164 TeaLeafIOS[251:13839] {tex} {tex} After: spritesheets/resources-images-menu_screen1.png canvas=0 access=1439358736
2015-08-12 11:22:17.164 TeaLeafIOS[251:13839] {tex} {tex} After: @TEXTAsap-Regular|35|255|255|255|255|274|0|0|Connect canvas=0 access=1439358736
2015-08-12 11:22:17.167 TeaLeafIOS[251:13839] {tex} Texture loaded: spritesheets/resources-images-map_screen0.png!  TOLOAD=0 USED=2162688
2015-08-12 11:22:17.172 TeaLeafIOS[251:13839] {tex} Texture loaded: spritesheets/resources-images-menu_screen0.png!  TOLOAD=0 USED=6356992
2015-08-12 11:22:17.177 TeaLeafIOS[251:13839] {tex} Texture loaded: spritesheets/resources-images-menu_screen1.png!  TOLOAD=0 USED=10551296
2015-08-12 11:22:17.182 TeaLeafIOS[251:13839] {tex} Texture loaded: spritesheets/resources-images-tutorial0.png!  TOLOAD=0 USED=12648448

@@ -750,7 +756,7 @@ void texture_manager_tick(texture_manager *manager) {

// zero the epoch used bins
memset(m_epoch_used, 0, sizeof(m_epoch_used));
} else if (highest > manager->max_texture_bytes) {
} else if (highest > manager->max_texture_bytes || overLimit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an issue in this change.

Trying to get more more memory when device has less memory available will kill the app.

@jishnu7
Copy link
Contributor

jishnu7 commented Oct 27, 2015

Sending my PR separately since my changes are independent of the area that @rogueSkib is talking about.

#13

@rogueSkib
Copy link
Contributor

Closing this, since we moved it to #12

@rogueSkib rogueSkib closed this Oct 29, 2015
@rampr rampr deleted the fix_texture_load_loop branch October 29, 2015 08:58
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.

3 participants