From 560523e817743f5ede020047aefd0b761d720790 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Mon, 6 Dec 2021 16:24:58 +0000 Subject: [PATCH] [FIX] mail: improve performance of listener removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avoid iterating over all listeners that are observing a field, instead store the fields observed by a listener and access it immediately. Part of task-2702450 closes odoo/odoo#81019 X-original-commit: bf69834b933c54598bbd82d8fbfcac4a0393c067 Signed-off-by: Alexandre Kühn (aku) Signed-off-by: Sébastien Theys (seb) --- .../mail/static/src/model/model_listener.js | 37 ++++++++++++++++++- addons/mail/static/src/model/model_manager.js | 36 ++++++++---------- 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/addons/mail/static/src/model/model_listener.js b/addons/mail/static/src/model/model_listener.js index 6642bef8248..a0038142c1f 100644 --- a/addons/mail/static/src/model/model_listener.js +++ b/addons/mail/static/src/model/model_listener.js @@ -30,9 +30,42 @@ export class Listener { this.isPartOfUpdateCycle = isPartOfUpdateCycle; this.name = name; this.onChange = onChange; - // Set of last observed elements, for debugging purposes. + /** + * Set of localIds that have been accessed on model manager between the + * last call to `startListening` and `stopListening` with this listener + * as parameter. + * Each localId has its own way to know the listeners that are observing + * it (to be able to notify them if it changes). This set holds the + * inverse of that information, which is useful to be able to remove + * this listener (when the need arises) from those localIds without + * having to verify the presence of this listener on each possible + * localId one by one. + */ this.lastObservedLocalIds = new Set(); - this.lastObservedFields = new Set(); + /** + * Map between localIds and a set of fields on those localIds that have + * been accessed on model manager between the last call to + * `startListening` and `stopListening` with this listener as parameter. + * Each field of each localId has its own way to know the listeners that + * are observing it (to be able to notify them if it changes). This map + * holds the inverse of that information, which is useful to be able to + * remove this listener (when the need arises) from those fields without + * having to verify the presence of this listener on each possible field + * one by one. + */ + this.lastObservedFieldsByLocalId = new Map(); + /** + * Set of Model that have been accessed with `all()` on model manager + * between the last call to `startListening` and `stopListening` with + * this listener as parameter. + * Each Model has its own way to know the listeners that are observing + * it (to be able to notify them if it changes). This set holds the + * inverse of that information, which is useful to be able to remove + * this listener (when the need arises) from those Model without having + * to verify the presence of this listener on each possible Model one by + * one. + */ + this.lastObservedAllByModel = new Set(); } /** diff --git a/addons/mail/static/src/model/model_manager.js b/addons/mail/static/src/model/model_manager.js index 5c53220445c..b08653151b0 100644 --- a/addons/mail/static/src/model/model_manager.js +++ b/addons/mail/static/src/model/model_manager.js @@ -89,12 +89,6 @@ export class ModelManager { * update cycle. Value contains list of info to help for debug. */ this._listenersToNotifyInUpdateCycle = new Map(); - /** - * Map between listeners and a set of localId that they are using. - * Useful for easily being able to clean up a listener without having to - * iterate all localId to be able to find which are using it. - */ - this._localIdsObservedByListener = new Map(); /** * All generated models. Keys are model name, values are model class. */ @@ -161,6 +155,7 @@ export class ModelManager { */ all(Model, filterFunc) { for (const listener of this._listeners) { + listener.lastObservedAllByModel.add(Model); const entry = this._listenersObservingAllByModel.get(Model); const info = { listener, @@ -257,8 +252,7 @@ export class ModelManager { throw Error(`wrong format of localId ${localId} for ${Model}.`); } for (const listener of this._listeners) { - this._localIdsObservedByListener.get(listener).add(localId); - listener.lastObservedLocalIds.add(this.localId); + listener.lastObservedLocalIds.add(localId); if (!this._listenersObservingLocalId.has(localId)) { this._listenersObservingLocalId.set(localId, new Map()); } @@ -347,18 +341,19 @@ export class ModelManager { this._listeners.delete(listener); this._listenersToNotifyInUpdateCycle.delete(listener); this._listenersToNotifyAfterUpdateCycle.delete(listener); - for (const localId of this._localIdsObservedByListener.get(listener) || []) { + for (const localId of listener.lastObservedLocalIds) { this._listenersObservingLocalId.get(localId).delete(listener); - if (this._listenersObservingFieldOfLocalId.has(localId)) { - for (const [, listenersUsingField] of this._listenersObservingFieldOfLocalId.get(localId)) { - listenersUsingField.delete(listener); - } + const listenersObservingFieldOfLocalId = this._listenersObservingFieldOfLocalId.get(localId); + for (const field of listener.lastObservedFieldsByLocalId.get(localId) || []) { + listenersObservingFieldOfLocalId.get(field).delete(listener); } } - this._localIdsObservedByListener.delete(listener); - for (const [, listeners] of this._listenersObservingAllByModel) { - listeners.delete(listener); + for (const Model of listener.lastObservedAllByModel) { + this._listenersObservingAllByModel.get(Model).delete(listener); } + listener.lastObservedLocalIds.clear(); + listener.lastObservedFieldsByLocalId.clear(); + listener.lastObservedAllByModel.clear(); } /** @@ -371,9 +366,6 @@ export class ModelManager { startListening(listener) { this.removeListener(listener); this._listeners.add(listener); - listener.lastObservedLocalIds.clear(); - listener.lastObservedFields.clear(); - this._localIdsObservedByListener.set(listener, new Set()); } /** @@ -1225,7 +1217,6 @@ export class ModelManager { Object.defineProperty(Model.prototype, field.fieldName, { get() { // this is bound to record for (const listener of this.modelManager._listeners) { - this.modelManager._localIdsObservedByListener.get(listener).add(this.localId); listener.lastObservedLocalIds.add(this.localId); if (!this.modelManager._listenersObservingLocalId.has(this.localId)) { this.modelManager._listenersObservingLocalId.set(this.localId, new Map()); @@ -1241,7 +1232,10 @@ export class ModelManager { entryLocalId.set(listener, [info]); } const entryField = this.modelManager._listenersObservingFieldOfLocalId.get(this.localId).get(field); - listener.lastObservedFields.add(field); + if (!listener.lastObservedFieldsByLocalId.has(this.localId)) { + listener.lastObservedFieldsByLocalId.set(this.localId, new Set()); + } + listener.lastObservedFieldsByLocalId.get(this.localId).add(field); if (entryField.has(listener)) { entryField.get(listener).push(info); } else {