Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
SUSE:SLE-12-SP2:GA
gnome-shell.2367
bnc963664-sle-background-memory-leaks-misc-fixe...
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File bnc963664-sle-background-memory-leaks-misc-fixes.patch of Package gnome-shell.2367
Index: gnome-shell-3.10.4/js/ui/background.js =================================================================== --- gnome-shell-3.10.4.orig/js/ui/background.js +++ gnome-shell-3.10.4/js/ui/background.js @@ -32,6 +32,10 @@ const ANIMATION_MIN_WAKEUP_INTERVAL = 1. let _backgroundCache = null; +// NOTE: the caching role is for various BMs like the normal desktop background, +// overview background, screen shield background, thumbnail backgrounds and etc, +// *NOT* for different backgrounds across selections. This is a major +// misunderstanding I had. const BackgroundCache = new Lang.Class({ Name: 'BackgroundCache', @@ -40,6 +44,7 @@ const BackgroundCache = new Lang.Class({ this._images = []; this._pendingFileLoads = []; this._fileMonitors = {}; + // NOTE: in recent GS, `_animations` is schema-keyed hash. this._animations = []; }, @@ -53,6 +58,12 @@ const BackgroundCache = new Lang.Class({ let content = null; let candidateContent = null; + // FIX: add exact match check to avoid unnecessary copy + // + // NOTE: the use of `candidateIsFullMatch` helps reduce the memory but + // the content removing code is dumb and might lead to premature content + // removal. + let candidateIsFullMatch = false; for (let i = 0; i < this._patterns.length; i++) { if (this._patterns[i].get_shading() != params.shadingType) continue; @@ -69,11 +80,17 @@ const BackgroundCache = new Lang.Class({ if (params.effects != this._patterns[i].effects) continue; + candidateIsFullMatch = true; break; } if (candidateContent) { - content = candidateContent.copy(params.monitorIndex, params.effects); + if (candidateIsFullMatch) { + content = candidateContent; + } else { + content = candidateContent.copy(params.monitorIndex, params.effects); + this._patterns.push(content); + } } else { content = new Meta.Background({ meta_screen: global.screen, monitor: params.monitorIndex, @@ -84,9 +101,11 @@ const BackgroundCache = new Lang.Class({ } else { content.load_gradient(params.shadingType, params.color, params.secondColor); } + + // FIX: push the new content, only when it's NEW + this._patterns.push(content); } - this._patterns.push(content); return content; }, @@ -112,6 +131,14 @@ const BackgroundCache = new Lang.Class({ monitor.disconnect(signalId); this.emit('file-changed', filename); + + // FIX: also remove this file monitor + // here. It should get removed while + // the related content is removed + // from `_images` but as noted in + // `_removeImageContent` there are + // some problems in relying on it. + delete this._fileMonitors[filename]; })); this._fileMonitors[filename] = monitor; @@ -124,15 +151,30 @@ const BackgroundCache = new Lang.Class({ contentList.splice(index, 1); }, + // FIX add animation cache management code, when background `_destroy`, it + // should call this function if it's an Animation. + removeAnimation: function(animation) { + delete this._fileMonitors[animation.filename]; + + this._animations = this._animations.filter(function(anime, idx){ + return anime !== animation; + }); + }, + removePatternContent: function(content) { this._removeContent(this._patterns, content); }, removeImageContent: function(content) { - let filename = content.get_filename(); - - if (filename && this._fileMonitors[filename]) - delete this._fileMonitors[filename]; + // TODO: one file can be loaded as different content and thus the + // following direct removal is too simplistic. We need to track refs to + // file to properly remove them. Here, we do NOT remove file watches (a + // leak). Enable this to sacrifice file watching in exchange of less + // memory drain see bnc#963664 (until we have proper ref count). + // + // let filename = content.get_filename(); + // if (filename && this._fileMonitors[filename]) + // delete this._fileMonitors[filename]; this._removeContent(this._images, content); }, @@ -143,6 +185,10 @@ const BackgroundCache = new Lang.Class({ if (!caller.cancellable) return; + // NOTE: connect to a signal `cancelled`, looks weird but seems to be + // valid, see https://www.roojs.com/seed/gir-1.2-gtk-3.0/gjs/Gio.Cancellable.html#expand + // + // This use of `connect` seems to be a special case caller.cancellable.connect(Lang.bind(this, function() { let idx = fileLoad.callers.indexOf(caller); fileLoad.callers.splice(idx, 1); @@ -183,39 +229,62 @@ const BackgroundCache = new Lang.Class({ cancellable: new Gio.Cancellable(), callers: [] }; this._attachCallerToFileLoad(caller, fileLoad); + // FIX: add this fileLoad to `_pendingFileLoads`, o/w we'll not be above + // to group callers together + this._pendingFileLoads.push(fileLoad); + // NOTE: do not set `monitorIndex` so later it will get re-copied with + // right caller info let content = new Meta.Background({ meta_screen: global.screen }); - content.load_file_async(params.filename, - params.style, - params.cancellable, - Lang.bind(this, - function(object, result) { - try { - content.load_file_finish(result); - - this._monitorFile(params.filename); - } catch(e) { - content = null; - } - - for (let i = 0; i < fileLoad.callers.length; i++) { - let caller = fileLoad.callers[i]; - if (caller.onFinished) { - let newContent; - - if (content) { - newContent = content.copy(caller.monitorIndex, caller.effects); - this._images.push(newContent); - } - - caller.onFinished(newContent); - } - } - - let idx = this._pendingFileLoads.indexOf(fileLoad); - this._pendingFileLoads.splice(idx, 1); - })); + content.load_file_async( + params.filename, + params.style, + params.cancellable, + Lang.bind(this, + function(object, result) { + try { + content.load_file_finish(result); + + this._monitorFile(params.filename); + } catch(e) { + content = null; + } + + // TODO: from my log, there is only one caller for fileLoad + // but this piece of code can run up to 6 times, with some of + // them having effects set to 2(Overview BG). This leads to + // multiple same entries in `cache._images` + + // FIX: a temporary area to avoid duplicates in cache `_images`. + let newContents = []; + let newContent = content; + for (let i = 0; i < fileLoad.callers.length; i++) { + let caller = fileLoad.callers[i]; + + if (content) { + newContent = content.copy(caller.monitorIndex, caller.effects); + + // only cache this content when other callers have NOT cached it. + let shouldCache = ! newContents.some(function(ctn) { + return ( (ctn.monitor === caller.monitorIndex) + && (ctn.effects === caller.effects) ); + }); + if (shouldCache) { + newContents.push(newContent); + this._images.push(newContent); + } + } + + // run `onFinished` despite caching condition + if (caller.onFinished) { + caller.onFinished(newContent); + } + } + + let idx = this._pendingFileLoads.indexOf(fileLoad); + this._pendingFileLoads.splice(idx, 1); + })); }, getImageContent: function(params) { @@ -229,6 +298,7 @@ const BackgroundCache = new Lang.Class({ let content = null; let candidateContent = null; + let candidateIsFullMatch = false; for (let i = 0; i < this._images.length; i++) { if (this._images[i].get_style() != params.style) continue; @@ -242,19 +312,24 @@ const BackgroundCache = new Lang.Class({ candidateContent = this._images[i]; + // NOTE `effects` field does exist, just not enlisted in the LookingGlass.... if (params.effects != this._images[i].effects) continue; + candidateIsFullMatch = true; break; } if (candidateContent) { - content = candidateContent.copy(params.monitorIndex, params.effects); + if (candidateIsFullMatch) { + content = candidateContent; + } else { + content = candidateContent.copy(params.monitorIndex, params.effects); + this._images.push(content); + } if (params.cancellable && params.cancellable.is_cancelled()) content = null; - else - this._images.push(content); if (params.onFinished) params.onFinished(content); @@ -276,25 +351,22 @@ const BackgroundCache = new Lang.Class({ let i; let animation = null; for (i = 0; i < this._animations.length; i++) { - if (this._animations[i].filename == params.filename) { + if (this._animations[i].filename === params.filename) { animation = this._animations[i]; - if (params.onLoaded) { - let id = GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() { - params.onLoaded(this._animation); - return GLib.SOURCE_REMOVE; - })); - GLib.Source.set_name_by_id(id, '[gnome-shell] params.onLoaded'); - } + + // FIX: a match has been found + break; } } if (animation == null) { animation = new Animation({ filename: params.filename }); this._animations.push(animation); + + // FIX: only monitor new animation's file + this._monitorFile(params.filename); } animation.load(Lang.bind(this, function() { - this._monitorFile(params.filename); - if (params.onLoaded) { let id = GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() { params.onLoaded(animation); @@ -336,12 +408,18 @@ const Background = new Lang.Class({ // contains a single image for static backgrounds and // two images (from and to) for slide shows this._images = {}; + // FIX: declare `_animation` here to avoid confusion + this._animation = null; + // FIX: declare here to avoid confusion + this._updateAnimationTimeoutId = 0; this._brightness = 1.0; this._vignetteSharpness = 0.2; this._cancellable = new Gio.Cancellable(); this.isLoaded = false; + // NOTE looks weird to me. shouldn't `changed` signal on settings be attached to BM instead? + // However, the end result is the same and should not affect functionality. this._settingsChangedSignalId = this._settings.connect('changed', Lang.bind(this, function() { this.emit('changed'); })); @@ -364,6 +442,10 @@ const Background = new Lang.Class({ } this._fileWatches = null; + if (this._animation) { + this._cache.removeAnimation(this._animation); + this._animation = null; + } if (this._pattern) { if (this._pattern.content) this._cache.removePatternContent(this._pattern.content); @@ -389,6 +471,9 @@ const Background = new Lang.Class({ if (this._settingsChangedSignalId != 0) this._settings.disconnect(this._settingsChangedSignalId); this._settingsChangedSignalId = 0; + + // FIX: reset `isLoaded` + this.isLoaded = false; }, _setLoaded: function() { @@ -397,10 +482,14 @@ const Background = new Lang.Class({ this.isLoaded = true; - GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() { + // NOTE: the extra waiting seems to avoid blocking the main loop as + // there might be multiple heavy handlers attached to `loaded` signal. + let id = GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() { this.emit('loaded'); - return false; + return GLib.SOURCE_REMOVE; })); + + GLib.Source.set_name_by_id(id, '[gnome-shell] this.emit'); }, _loadPattern: function() { @@ -438,6 +527,7 @@ const Background = new Lang.Class({ this._fileWatches[filename] = signalId; }, + // NOTE maybe more properly, `_ensureImageActor` _ensureImage: function(index) { if (this._images[index]) return; @@ -450,13 +540,14 @@ const Background = new Lang.Class({ this._images[index] = actor; }, + // NOTE maybe more properly, `_updateImageContent` _updateImage: function(index, content, filename) { content.brightness = this._brightness; content.vignette_sharpness = this._vignetteSharpness; let image = this._images[index]; if (image.content) - this._cache.removeImageContent(content); + this._cache.removeImageContent(image.content); image.content = content; this._watchCacheFile(filename); }, @@ -536,29 +627,33 @@ const Background = new Lang.Class({ if (interval > GLib.MAXUINT32) return; - this._updateAnimationTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, - interval, - Lang.bind(this, function() { - this._updateAnimationTimeoutId = 0; - this._updateAnimation(); - return false; - })); + this._updateAnimationTimeoutId = GLib.timeout_add( + GLib.PRIORITY_DEFAULT, + interval, + Lang.bind(this, function() { + this._updateAnimationTimeoutId = 0; + this._updateAnimation(); + return GLib.SOURCE_REMOVE; + })); + + GLib.Source.set_name_by_id(this._updateAnimationTimeoutId, + '[gnome-shell] this._updateAnimation'); }, _loadAnimation: function(filename) { this._cache.getAnimation({ filename: filename, - onLoaded: Lang.bind(this, function(animation) { - this._animation = animation; + onLoaded: Lang.bind(this, function(animation) { + this._animation = animation; - if (!this._animation || this._cancellable.is_cancelled()) { - this._setLoaded(); - return; - } - - this._updateAnimation(); - this._watchCacheFile(filename); - }) - }); + if (!this._animation || this._cancellable.is_cancelled()) { + this._setLoaded(); + return; + } + + this._updateAnimation(); + this._watchCacheFile(filename); + }) + }); }, _loadImage: function(filename) { @@ -587,8 +682,9 @@ const Background = new Lang.Class({ _load: function () { this._cache = getBackgroundCache(); - this._loadPattern(this._cache); + this._loadPattern(); + // NOTE: the following says: no image will be used so only show the pattern this._style = this._settings.get_enum(BACKGROUND_STYLE_KEY); if (this._style == GDesktopEnums.BackgroundStyle.NONE) { this._setLoaded(); @@ -666,7 +762,11 @@ const SystemBackground = new Lang.Class( }, _onDestroy: function() { - this._cache.removeImageContent(this.actor.content); + // FIX: upstream cfef107114 + let content = this.actor.content; + + if (content) + this._cache.removeImageContent(content); }, }); Signals.addSignalMethods(SystemBackground.prototype); @@ -755,30 +855,39 @@ const BackgroundManager = new Lang.Class } }, - _updateBackground: function(background) { + _updateBackground: function() { let newBackground = this._createBackground(); - newBackground.vignetteSharpness = background.vignetteSharpness; - newBackground.brightness = background.brightness; - newBackground.visible = background.visible; + newBackground.vignetteSharpness = this.background.vignetteSharpness; + newBackground.brightness = this.background.brightness; + newBackground.visible = this.background.visible; newBackground.loadedSignalId = newBackground.connect('loaded', Lang.bind(this, function() { newBackground.disconnect(newBackground.loadedSignalId); newBackground.loadedSignalId = 0; - Tweener.addTween(background.actor, + // NOTE: new background is loaded *asynchronously*, while + // `this._newBackground` holds the *newest* background. The + // following check make sure what gets loaded is still relevant. + // + // Backported from upstream commits: 933f38390ba1512aa and d6197b0904fa. + if (this._newBackground != newBackground) { + /* Not interesting, we queued another load */ + newBackground.actor.destroy(); + return; + } + Tweener.addTween(this.background.actor, { opacity: 0, time: FADE_ANIMATION_TIME, transition: 'easeOutQuad', onComplete: Lang.bind(this, function() { - if (this._newBackground == newBackground) { - this.background = newBackground; - this._newBackground = null; - } else { - newBackground.actor.destroy(); - } - - background.actor.destroy(); - + this.background.actor.destroy(); + this.background = newBackground; + this._newBackground = null; + + // FIX: "changed" signal is emitted + // and old background destroyed *only + // when* changes really happen + // see c#24 for the discussion. this.emit('changed'); }) }); @@ -805,13 +914,14 @@ const BackgroundManager = new Lang.Class background.changeSignalId = background.connect('changed', Lang.bind(this, function() { background.disconnect(background.changeSignalId); background.changeSignalId = 0; - this._updateBackground(background); + this._updateBackground(); })); background.actor.connect('destroy', Lang.bind(this, function() { if (background.changeSignalId) background.disconnect(background.changeSignalId); + // NOTE: loadedSignalId is set in _updateBackground if (background.loadedSignalId) background.disconnect(background.loadedSignalId); })); Index: gnome-shell-3.10.4/src/shell-global.c =================================================================== --- gnome-shell-3.10.4.orig/src/shell-global.c +++ gnome-shell-3.10.4/src/shell-global.c @@ -1417,6 +1417,13 @@ run_leisure_functions (gpointer data) if (global->work_count > 0) return FALSE; + /* MEM-FIX: run gc at tweener's end, this reduces the performance but avoid memory leaks + * see http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/wily/gnome-shell/wily/view/head:/debian/patches/revert-disable-periodic-gc.patch + * + * NOTE: a bit overkill to fix bg memory problem, what I want is simply run gc after every background change not every tweener. + */ + gjs_context_gc (global->js_context); + /* No leisure closures, so we are done */ if (global->leisure_closures == NULL) return FALSE;
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
