Adds tests and potential fixes for #10771 and #11143 #11291
Conversation
Unfortunately a brief spin of the run loop is required for the layer reuse test. I've tried to tune this down for my machine; YMMV
akitchen
added
iOS
|
@asheemmamoowala @julianrex I have a potential fix for these errors (and integration tests not running) at gl-layer-memory-fix...jrex-gl-layer-memory-fix. All the tests run, and I played with the iOS app for a while and all seemed good, but obviously this could have some significant impact, and there may be other cases that I haven't considered. Can you take a look? If it looks good, we could merge into master, and then cherry-pick into boba? To get the tests to run on device, I had to add Mapbox.framework to the test app as an embedded framework (rather than adding it to the integration framework). |
|
Added a PR into your branch @akitchen. |
… to styles. Integration tests run on device.
…he style. Update associated test that failed.
akitchen
changed the title
akitchen
removed
the
⚠️ DO NOT MERGE
|
@jmkiley do you have a working example which we could use to verify these fixes? I thought maybe you did, from your previous investigations... |
|
|
||
| // Pair the retain above, and release self, since we're now removed from the collection | ||
| CFTypeRef toRelease = (__bridge CFTypeRef)self; | ||
| CFBridgingRelease(toRelease); |
There was a problem hiding this comment.
Now I'm wondering - is this guaranteed to happen, always? Or do we need more of a dance to ensure this is released if the whole style is destroyed without an explicit call to -removeFromStyle:...
There was a problem hiding this comment.
I believe so, when the style gets cleaned up. But we should add a test for it.
There was a problem hiding this comment.
Scratch that - I think we need some more clean-up.
|
I used this branch with my repro branch and it seemed to be 👍 |
…leChanged is failing
akitchen
added
the
⚠️ DO NOT MERGE
| // this is because although the layer has been removed from the style, there's still a pending | ||
| // render (deinitialize) call, that will needs to be handled, which will finally release the | ||
| // layer (and then the layer will be dealloc'd when the autorelease pool drains) | ||
| [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; |
There was a problem hiding this comment.
If we expect the layer to be released as of the next render, how about putting in an expectation that gets satisfied upon the next call to -[MGLMapViewDelegate mapViewDidFinishRenderingFrame:fullyRendered:]?
|
Incorporated into: #11553 |
|
I believe this can be closed now that #11553 has landed... please reopen if I'm mistaken. |
Unfortunately a brief spin of the run loop is required for some of these tests; we should take a critical look at which ones are worth keeping around once we're satisfied with the fixes they are helping to drive, in order to avoid slowing down our test suite too much.
/ref #10771 #11143
/cc @asheemmamoowala @julianrex @1ec5 @lilykaiser