From f7b0f6750be73bddcb150b24bb46942dddb383db Mon Sep 17 00:00:00 2001 From: jahow Date: Wed, 30 Oct 2019 21:59:57 +0100 Subject: [PATCH] Resolve memory leak when deleting a webgl layer Various references were kept, preventing the layer and underlying renderer and webgl context to be garbage collected. Also, the Helper was simplified because it turns out deleting manually all Webgl objects is useless: these objects will be released when the context is garbage collected anyway. Note: this touches the Layer and BaseLayer classes, as the following were preventing the layer from being garbage collected: * layer reference in the `state_` object in BaseLayer * dangling listener for source change in Layer --- examples/webgl-points-layer.js | 1 + src/ol/layer/Base.js | 11 ++++++++++ src/ol/layer/Layer.js | 7 +++++++ src/ol/layer/WebGLPoints.js | 12 +++++++++++ src/ol/renderer/webgl/Layer.js | 2 +- src/ol/renderer/webgl/PointsLayer.js | 15 ++++++++++---- src/ol/webgl/Helper.js | 30 ---------------------------- 7 files changed, 43 insertions(+), 35 deletions(-) diff --git a/examples/webgl-points-layer.js b/examples/webgl-points-layer.js index ae04790a71..3d3c980a41 100644 --- a/examples/webgl-points-layer.js +++ b/examples/webgl-points-layer.js @@ -124,6 +124,7 @@ function refreshLayer(newStyle) { if (previousLayer) { map.removeLayer(previousLayer); + previousLayer.dispose(); } literalStyle = newStyle; } diff --git a/src/ol/layer/Base.js b/src/ol/layer/Base.js index 7155040289..21a44fdc36 100644 --- a/src/ol/layer/Base.js +++ b/src/ol/layer/Base.js @@ -320,6 +320,17 @@ class BaseLayer extends BaseObject { setZIndex(zindex) { this.set(LayerProperty.Z_INDEX, zindex); } + + /** + * @inheritDoc + */ + disposeInternal() { + if (this.state_) { + this.state_.layer = null; + this.state_ = null; + } + super.disposeInternal(); + } } diff --git a/src/ol/layer/Layer.js b/src/ol/layer/Layer.js index 0291785b19..bef87a803e 100644 --- a/src/ol/layer/Layer.js +++ b/src/ol/layer/Layer.js @@ -287,6 +287,13 @@ class Layer extends BaseLayer { return null; } + /** + * @inheritDoc + */ + disposeInternal() { + this.setSource(null); + super.disposeInternal(); + } } diff --git a/src/ol/layer/WebGLPoints.js b/src/ol/layer/WebGLPoints.js index c3bdb80b10..a49ff9e21e 100644 --- a/src/ol/layer/WebGLPoints.js +++ b/src/ol/layer/WebGLPoints.js @@ -57,6 +57,9 @@ import Layer from './Layer.js'; * } * ``` * + * **Important: a `WebGLPoints` layer must be manually disposed when removed, otherwise the underlying WebGL context + * will not be garbage collected.** + * * Note that any property set in the options is set as a {@link module:ol/Object~BaseObject} * property on the layer object; for example, setting `title: 'My Title'` in the * options means that `title` is observable, and has get/set accessors. @@ -100,6 +103,15 @@ class WebGLPointsLayer extends Layer { attributes: this.parseResult_.attributes }); } + + /** + * + * @inheritDoc + */ + disposeInternal() { + this.renderer_.dispose(); + super.disposeInternal(); + } } export default WebGLPointsLayer; diff --git a/src/ol/renderer/webgl/Layer.js b/src/ol/renderer/webgl/Layer.js index f7f1848268..b3f6072fec 100644 --- a/src/ol/renderer/webgl/Layer.js +++ b/src/ol/renderer/webgl/Layer.js @@ -71,7 +71,7 @@ class WebGLLayerRenderer extends LayerRenderer { * @inheritDoc */ disposeInternal() { - this.helper.disposeInternal(); + this.helper.dispose(); super.disposeInternal(); } diff --git a/src/ol/renderer/webgl/PointsLayer.js b/src/ol/renderer/webgl/PointsLayer.js index 7e8ddb7874..f0dbdb7452 100644 --- a/src/ol/renderer/webgl/PointsLayer.js +++ b/src/ol/renderer/webgl/PointsLayer.js @@ -19,7 +19,7 @@ import {getUid} from '../../util.js'; import WebGLRenderTarget from '../../webgl/RenderTarget.js'; import {assert} from '../../asserts.js'; import BaseVector from '../../layer/BaseVector.js'; -import {listen} from '../../events.js'; +import {listen, unlistenByKey} from '../../events.js'; import VectorEventType from '../../source/VectorEventType.js'; /** @@ -288,9 +288,11 @@ class WebGLPointsLayerRenderer extends WebGLLayerRenderer { this.featureCache_ = {}; const source = this.getLayer().getSource(); - listen(source, VectorEventType.ADDFEATURE, this.handleSourceFeatureChanged_, this); - listen(source, VectorEventType.CHANGEFEATURE, this.handleSourceFeatureChanged_, this); - listen(source, VectorEventType.REMOVEFEATURE, this.handleSourceFeatureDelete_, this); + this.sourceListenKeys_ = [ + listen(source, VectorEventType.ADDFEATURE, this.handleSourceFeatureChanged_, this), + listen(source, VectorEventType.CHANGEFEATURE, this.handleSourceFeatureChanged_, this), + listen(source, VectorEventType.REMOVEFEATURE, this.handleSourceFeatureDelete_, this) + ]; source.getFeatures().forEach(function(feature) { const uid = getUid(feature); this.featureCache_[uid] = { @@ -548,6 +550,11 @@ class WebGLPointsLayerRenderer extends WebGLLayerRenderer { */ disposeInternal() { this.worker_.terminate(); + this.layer_ = null; + this.sourceListenKeys_.forEach(function(key) { + unlistenByKey(key); + }); + this.sourceListenKeys_ = null; super.disposeInternal(); } } diff --git a/src/ol/webgl/Helper.js b/src/ol/webgl/Helper.js index b44944b4ff..27434f79ea 100644 --- a/src/ol/webgl/Helper.js +++ b/src/ol/webgl/Helper.js @@ -266,18 +266,6 @@ class WebGLHelper extends Disposable { */ this.bufferCache_ = {}; - /** - * @private - * @type {!Array} - */ - this.shaderCache_ = []; - - /** - * @private - * @type {!Array} - */ - this.programCache_ = []; - /** * @private * @type {WebGLProgram} @@ -419,18 +407,6 @@ class WebGLHelper extends Disposable { disposeInternal() { this.canvas_.removeEventListener(ContextEventType.LOST, this.boundHandleWebGLContextLost_); this.canvas_.removeEventListener(ContextEventType.RESTORED, this.boundHandleWebGLContextRestored_); - const gl = this.getGL(); - if (!gl.isContextLost()) { - for (const key in this.bufferCache_) { - gl.deleteBuffer(this.bufferCache_[key].buffer); - } - for (const key in this.programCache_) { - gl.deleteProgram(this.programCache_[key]); - } - for (const key in this.shaderCache_) { - gl.deleteShader(this.shaderCache_[key]); - } - } } /** @@ -652,7 +628,6 @@ class WebGLHelper extends Disposable { const shader = gl.createShader(type); gl.shaderSource(shader, source); gl.compileShader(shader); - this.shaderCache_.push(shader); return shader; } @@ -684,7 +659,6 @@ class WebGLHelper extends Disposable { gl.attachShader(program, fragmentShader); gl.attachShader(program, vertexShader); gl.linkProgram(program); - this.programCache_.push(program); return program; } @@ -811,8 +785,6 @@ class WebGLHelper extends Disposable { */ handleWebGLContextLost() { clear(this.bufferCache_); - clear(this.shaderCache_); - clear(this.programCache_); this.currentProgram_ = null; } @@ -823,8 +795,6 @@ class WebGLHelper extends Disposable { handleWebGLContextRestored() { } - // TODO: shutdown program - /** * Will create or reuse a given webgl texture and apply the given size. If no image data * specified, the texture will be empty, otherwise image data will be used and the `size`