Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Use a pair of NSSets to manage the retain/release lifecycle of layers. #11343

Closed

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Feb 28, 2018

(Obj-C) Layers are not retained at all by the SDK, but the C++ Layers can reach back to them (especially as custom layer contexts). I think there needs to be some mechanism that holds on to the Obj-C layers, regardless of what the client app does.

This PR adds a set (to MGLStyle) to which these layers are added, purely to manage the retain counts. When rendering starts, this set is copied, so that their lifetime is guaranteed (and so those contexts was become garbage).

(It's possible that the initial copy might need to move from rendering start to being triggered Map::Impl::onUpdate())

(This addresses #11143)

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

These changes do not address the scenario in testOpenGLLayerDoesNotLeakWhenStyleChanged. (It's possible the test is passing, but I believe the underlying issue is not addressed here).

When the style is changed with a call like [mapView setStyleURL:styleURL], the core layer is eventually cleaned up, but there will not be a corresponding call to [MGLStyle removeFromManagedLayers].

One possible way to capture this case is to handle the StyleObserver::OnStyleLoaded callback and check if any layer ids have been removed. To implement this, the NSMutableSet will need to be changed to an NSMutableDictionary so that the layer identifier does not have to be compared against the id in the core layer, which may already have been deleted.

@@ -534,6 +540,25 @@ - (void)insertLayer:(MGLStyleLayer *)layer aboveLayer:(MGLStyleLayer *)sibling {
[self didChangeValueForKey:@"layers"];
}

#pragma mark - Layer retain/release management

- (void)addToManagedLayers:(MGLStyleLayer*)layer {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be invoked from [MGLStyle layerFromMBGLLayer] as well, to capture internally created style layers.

@@ -85,6 +85,10 @@ @interface MGLStyle()
@property (readonly, copy, nullable) NSURL *URL;
@property (nonatomic) NS_MUTABLE_DICTIONARY_OF(NSString *, NS_DICTIONARY_OF(NSObject *, MGLTextLanguage *) *) *localizedLayersByIdentifier;

// Used for retain/release management
@property (nonatomic) NSMutableSet *layersForUpdating;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the use of these properties remove the need for tracking the peer object in core for style layers? For layers created in the SDK, [MGLStlye layerFromMBGLLayer] could be used to lookup this set instead of querying core

Julian Rex added 2 commits March 5, 2018 13:16
…MGLOpenGLStyleLayers for the 2 trips through the render loop that are needed before they can be deallocated. Added additional tests.
@julianrex julianrex added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Mar 5, 2018
@julianrex
Copy link
Contributor Author

The latest changes have evolved somewhat from the initial PR:

  • An MGLStyle still holds on to MGLOpenGLStyleLayers (but now restricted to just that type).
  • The MGLMapView has an object that manages the retain/release for the layers that a referenced from RenderLayer (in fact their Impls). This object uses a lifetime mechanism to know when to remove the object from its registry. The lifetimes are reset (using the MGLStyle's set) at the beginning of the render process, and decremented at the end. This handles the case where the style can be changed mid-flight.
  • I've had to add a deallocation callback to mbgl::style::CustomLayer which isn't ideal - so that the MGLOpenGLStyleLayer can clear its unmanaged rawLayer pointer (via the peer object). As I understand it this affects all platforms, e.g. QT, Android - I have not yet made the corresponding change there.
  • Additional tests have been created and existing ones beefed up to hopefully catch all the possible scenarios.

@julianrex
Copy link
Contributor Author

Closing in preference of #11553

@julianrex julianrex closed this Apr 2, 2018
@jfirebaugh jfirebaugh deleted the jrex-custom-layer-style-changed-leak branch July 27, 2018 22:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants