From a9e546b19f81a7c3d58d57611dc3d5b9ba93d144 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 13:38:46 +0200 Subject: [PATCH 01/21] Add 'per-instance' cacheing of member-methods (results) functionality The usual functools.lru_cache mechanism for cacheing function-results runs into a number of problems when applied to member-methods of class-instances (class-methods are unaffectd by all of those). If relatively easily possible however, use of build-ins is still preferred over alternatives. In this commit, a wrapper around the basic lru_cache is introduced that makes it possible to avoid the main problem that we had with its usage; clearing out the cache would be 'per function', not 'per instance of a class that function belongs to', with the result that if you try to clear out the cache for (all functions of) that instance, you instead clear out the cache belonging to _all_ instances of that class (for all functions). This is because classes in Python are (conceptually at least) 'just' syntactic sugar, and (thus) the self-argument is 'just another argument' to a function that is 'static' of sorts 'underwater'. This wrapper prevents that, by making a partial-application function out of the function and its accociated instance, which in turn allows the lru-cache to 'just work'. A singleton is then provided to access functionality for clearing out the cache for a specific instance. Problems that still need to be solved are a.) garbage-collection of deleted instances, since we now keep a reference to the class (which should be easy) -- and b.) what to do with keyword arguments ('kwargs'), since they get passed to the function-cache as a dictionary, and can't be used as arguments (anymore) because lists, sets and dicts aren't normally hashable (which might prove more of a challenge -- we might have to content with just not cacheing functions that have lists or dicts as arguments, or split those functions up to facilitate cacheing of intermediate results instead). forms the basis for CURA-11761 --- UM/Decorators.py | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/UM/Decorators.py b/UM/Decorators.py index e48ba86157..1865226004 100644 --- a/UM/Decorators.py +++ b/UM/Decorators.py @@ -1,7 +1,8 @@ -# Copyright (c) 2018 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. import copy +from functools import lru_cache, partial import warnings import inspect @@ -137,4 +138,38 @@ def timed(*args, **kw): print("Function %r took %2.2f ms" % (method.__name__, (te - ts) * 1000)) return result - return timed \ No newline at end of file + return timed + + +class CachedMemberFunctions: + __instance = None + + @classmethod + def getInstance(cls): + if cls.__instance is None: + cls.__instance = cls() + return cls.__instance + + @classmethod + def clearInstanceCache(cls, instance): + cls.getInstance()._cache[instance] = {} + + def __init__(self): + if CachedMemberFunctions.__instance is not None: + raise RuntimeError(f"Attempt to instantiate singleton '{self.__class__.__name__}' more than once.") + self._cache = {} + + def callFunction(self, instance, function, *args, **kwargs): + # FIXME: Keyword arguments ('kwargs') will probably not _actually_ work here, since it's given as a dictionary, + # and the arguments need to be hashable in order for it to properly fit into the lru_cache. + if instance not in self._cache: + self._cache[instance] = {} + if function not in self._cache[instance]: + self._cache[instance][function] = lru_cache()(partial(function, instance)) + return self._cache[instance][function](*args, **kwargs) + + +def cachePerInstance(function): + def wrapper(instance, *args, **kwargs): + return CachedMemberFunctions.getInstance().callFunction(instance, function, *args, **kwargs) + return wrapper From 5ddb85835d697c1e6869fdfa44993de896bd9037 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 13:43:12 +0200 Subject: [PATCH 02/21] Use the newly introduced caching wrapper for container-stacks. Not using lru_cache here (expect for class methods), so we can clear the cache per class-instance instead of 'semi-globally' (per function). Splitting up function(s) was done because we still use lru_cache under the hood, and that (and the wrapper at the moment) can't handle dict, list, or set arguements, because those aren't hashable. contributes to CURA-11761 --- UM/Settings/ContainerStack.py | 49 ++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/UM/Settings/ContainerStack.py b/UM/Settings/ContainerStack.py index 238488b127..d8f7282dd9 100644 --- a/UM/Settings/ContainerStack.py +++ b/UM/Settings/ContainerStack.py @@ -1,8 +1,9 @@ -# Copyright (c) 2022 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. import configparser import io +from functools import lru_cache from typing import Any, cast, Dict, List, Optional, Set, Tuple from PyQt6.QtCore import QObject, pyqtProperty, pyqtSignal @@ -12,6 +13,7 @@ import UM.FlameProfiler from UM.ConfigurationErrorMessage import ConfigurationErrorMessage +from UM.Decorators import CachedMemberFunctions, cachePerInstance from UM.Signal import Signal, signalemitter from UM.PluginObject import PluginObject from UM.MimeTypeDatabase import MimeTypeDatabase, MimeType @@ -94,6 +96,7 @@ def __getstate__(self) -> Dict[str, Any]: def __setstate__(self, state: Dict[str, Any]) -> None: """For pickle support""" + CachedMemberFunctions.clearInstanceCache(self) self.__dict__.update(state) def getId(self) -> str: @@ -121,6 +124,7 @@ def setName(self, name: str) -> None: """ if name != self.getName(): + CachedMemberFunctions.clearInstanceCache(self) self._metadata["name"] = name self._dirty = True self.nameChanged.emit() @@ -141,6 +145,7 @@ def isReadOnly(self) -> bool: def setReadOnly(self, read_only: bool) -> None: if read_only != self._read_only: + CachedMemberFunctions.clearInstanceCache(self) self._read_only = read_only self.readOnlyChanged.emit() @@ -160,6 +165,7 @@ def setMetaData(self, meta_data: Dict[str, Any]) -> None: if meta_data == self.getMetaData(): return #Unnecessary. + CachedMemberFunctions.clearInstanceCache(self) #We'll fill a temporary dictionary with all the required metadata and overwrite it with the new metadata. #This way it is ensured that at least the required metadata is still there. @@ -176,12 +182,8 @@ def setMetaData(self, meta_data: Dict[str, Any]) -> None: metaDataChanged = pyqtSignal(QObject) metaData = pyqtProperty("QVariantMap", fget = getMetaData, fset = setMetaData, notify = metaDataChanged) - def getMetaDataEntry(self, entry: str, default: Any = None) -> Any: - """:copydoc ContainerInterface::getMetaDataEntry - - Reimplemented from ContainerInterface - """ - + @cachePerInstance + def _getRawMetaDataEntry(self, entry: str) -> Any: value = self._metadata.get(entry, None) if value is None: @@ -190,6 +192,15 @@ def getMetaDataEntry(self, entry: str, default: Any = None) -> Any: if value is not None: break + return value + + def getMetaDataEntry(self, entry: str, default: Any = None) -> Any: + """:copydoc ContainerInterface::getMetaDataEntry + + Reimplemented from ContainerInterface + """ + + value = self._getRawMetaDataEntry(entry) if value is None: return default else: @@ -197,12 +208,14 @@ def getMetaDataEntry(self, entry: str, default: Any = None) -> Any: def setMetaDataEntry(self, key: str, value: Any) -> None: if key not in self._metadata or self._metadata[key] != value: + CachedMemberFunctions.clearInstanceCache(self) self._metadata[key] = value self._dirty = True self.metaDataChanged.emit(self) def removeMetaDataEntry(self, key: str) -> None: if key in self._metadata: + CachedMemberFunctions.clearInstanceCache(self) del self._metadata[key] self.metaDataChanged.emit(self) @@ -210,10 +223,12 @@ def isDirty(self) -> bool: return self._dirty def setDirty(self, dirty: bool) -> None: + CachedMemberFunctions.clearInstanceCache(self) self._dirty = dirty containersChanged = Signal() + @cachePerInstance def getProperty(self, key: str, property_name: str, context: Optional[PropertyEvaluationContext] = None) -> Any: """:copydoc ContainerInterface::getProperty @@ -240,6 +255,7 @@ def getProperty(self, key: str, property_name: str, context: Optional[PropertyEv return value + @cachePerInstance def getRawProperty(self, key: str, property_name: str, *, context: Optional[PropertyEvaluationContext] = None, use_next: bool = True, skip_until_container: Optional[ContainerInterface] = None) -> Any: """Retrieve a property of a setting by key and property name. @@ -289,6 +305,7 @@ def getRawProperty(self, key: str, property_name: str, *, context: Optional[Prop else: return None + @cachePerInstance def hasProperty(self, key: str, property_name: str) -> bool: """:copydoc ContainerInterface::hasProperty @@ -346,6 +363,7 @@ def serialize(self, ignored_metadata_keys: Optional[Set[str]] = None) -> str: return stream.getvalue() @classmethod + @lru_cache def _readAndValidateSerialized(cls, serialized: str) -> configparser.ConfigParser: """Deserializes the given data and checks if the required fields are present. @@ -362,6 +380,7 @@ def _readAndValidateSerialized(cls, serialized: str) -> configparser.ConfigParse return parser @classmethod + @lru_cache def getConfigurationTypeFromSerialized(cls, serialized: str) -> Optional[str]: configuration_type = None try: @@ -374,6 +393,7 @@ def getConfigurationTypeFromSerialized(cls, serialized: str) -> Optional[str]: return configuration_type @classmethod + @lru_cache def getVersionFromSerialized(cls, serialized: str) -> Optional[int]: configuration_type = cls.getConfigurationTypeFromSerialized(serialized) if not configuration_type: @@ -389,6 +409,7 @@ def getVersionFromSerialized(cls, serialized: str) -> Optional[int]: Logger.log("d", "Could not get version from serialized: %s", e) return version + @cachePerInstance def deserialize(self, serialized: str, file_name: Optional[str] = None) -> str: """:copydoc ContainerInterface::deserialize @@ -452,6 +473,7 @@ def deserialize(self, serialized: str, file_name: Optional[str] = None) -> str: return serialized @classmethod + @lru_cache def deserializeMetadata(cls, serialized: str, container_id: str) -> List[Dict[str, Any]]: """Gets the metadata of a container stack from a serialised format. @@ -485,6 +507,7 @@ def deserializeMetadata(cls, serialized: str, container_id: str) -> List[Dict[st return [metadata] + @cachePerInstance def getAllKeys(self) -> Set[str]: """Get all keys known to this container stack. @@ -502,6 +525,7 @@ def getAllKeys(self) -> Set[str]: keys |= self._next_stack.getAllKeys() return keys + @cachePerInstance def getAllKeysWithUserState(self) -> Set[str]: """Get a subset of all the settings keys in all the containers having a User state @@ -519,6 +543,7 @@ def getAllKeysWithUserState(self) -> Set[str]: return settings + @cachePerInstance def getContainers(self) -> List[ContainerInterface]: """Get a list of all containers in this stack. @@ -586,8 +611,10 @@ def setPath(self, path: str) -> None: Reimplemented from ContainerInterface """ + CachedMemberFunctions.clearInstanceCache(self) self._path = path + @cachePerInstance def getSettingDefinition(self, key: str) -> Optional[SettingDefinition]: """Get the SettingDefinition object for a specified key""" @@ -605,6 +632,7 @@ def getSettingDefinition(self, key: str) -> Optional[SettingDefinition]: return None @UM.FlameProfiler.profile + #TODO: Find a way around keyword-arguments being passed as a dict, or live with not cacheing this particular func. def findContainer(self, criteria: Dict[str, Any] = None, container_type: type = None, **kwargs: Any) -> Optional[ContainerInterface]: """Find a container matching certain criteria. @@ -648,6 +676,7 @@ def addContainer(self, container: ContainerInterface) -> None: :param container: The container to add to the stack. """ + CachedMemberFunctions.clearInstanceCache(self) self.insertContainer(0, container) def insertContainer(self, index: int, container: ContainerInterface) -> None: @@ -661,6 +690,7 @@ def insertContainer(self, index: int, container: ContainerInterface) -> None: if container is self: raise Exception("Unable to add stack to itself.") + CachedMemberFunctions.clearInstanceCache(self) container.propertyChanged.connect(self._collectPropertyChanges) self._containers.insert(index, container) self.containersChanged.emit(container) @@ -682,6 +712,7 @@ def replaceContainer(self, index: int, container: ContainerInterface, postpone_e if container is self: raise Exception("Unable to replace container with ContainerStack (self) ") + CachedMemberFunctions.clearInstanceCache(self) self._containers[index].propertyChanged.disconnect(self._collectPropertyChanges) container.propertyChanged.connect(self._collectPropertyChanges) self._containers[index] = container @@ -703,6 +734,7 @@ def removeContainer(self, index: int = 0) -> None: if index < 0: raise IndexError try: + CachedMemberFunctions.clearInstanceCache(self) container = self._containers[index] self._dirty = True container.propertyChanged.disconnect(self._collectPropertyChanges) @@ -735,6 +767,7 @@ def setNextStack(self, stack: "ContainerStack", connect_signals: bool = True) -> if self._next_stack == stack: return + CachedMemberFunctions.clearInstanceCache(self) if self._next_stack: self._next_stack.propertyChanged.disconnect(self._collectPropertyChanges) self.containersChanged.disconnect(self._next_stack.containersChanged) @@ -754,6 +787,7 @@ def sendPostponedEmits(self) -> None: signal.emit(signal_arg) @UM.FlameProfiler.profile + @cachePerInstance def hasErrors(self) -> bool: """Check if the container stack has errors""" @@ -776,6 +810,7 @@ def hasErrors(self) -> bool: return False @UM.FlameProfiler.profile + @cachePerInstance def getErrorKeys(self) -> List[str]: """Get all the keys that are in an error state in this stack""" From cd28092fd695789b0eefbf4c7341bfcd41e10b8b Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 14:00:52 +0200 Subject: [PATCH 03/21] 'Re-enable' garbage collection on objects with cached members. Otherwise, a reference to the object still exists, which may prevent it from being garbage-collected. part of CURA-11761 --- UM/Decorators.py | 4 ++++ UM/Settings/ContainerStack.py | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/UM/Decorators.py b/UM/Decorators.py index 1865226004..c5679f5e90 100644 --- a/UM/Decorators.py +++ b/UM/Decorators.py @@ -154,6 +154,10 @@ def getInstance(cls): def clearInstanceCache(cls, instance): cls.getInstance()._cache[instance] = {} + @classmethod + def deleteInstanceCache(cls, instance): + del cls.getInstance()._cache[instance] + def __init__(self): if CachedMemberFunctions.__instance is not None: raise RuntimeError(f"Attempt to instantiate singleton '{self.__class__.__name__}' more than once.") diff --git a/UM/Settings/ContainerStack.py b/UM/Settings/ContainerStack.py index d8f7282dd9..f06603416c 100644 --- a/UM/Settings/ContainerStack.py +++ b/UM/Settings/ContainerStack.py @@ -867,6 +867,11 @@ def __str__(self) -> str: def __repr__(self) -> str: return str(self) + def __del__(self) -> None: + # None of the 'parents' seem to have __dell__, so OK not to call `super.del` here. + CachedMemberFunctions.deleteInstanceCache(self) + + _containerRegistry = ContainerRegistryInterface() # type: ContainerRegistryInterface From 649c92ccd20f9123c9e5678e4d5185856c6e3be5 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 15:51:00 +0200 Subject: [PATCH 04/21] Make tests work on Windows again. Database connection wasn't closed, which caused the file to be marked as 'in use' on Windows. sort-of done as part of CURA-11761 --- tests/Settings/conftest.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Settings/conftest.py b/tests/Settings/conftest.py index 9d2e8bd41a..adb956fa4b 100644 --- a/tests/Settings/conftest.py +++ b/tests/Settings/conftest.py @@ -50,6 +50,9 @@ def container_registry(application, test_containers_provider, plugin_registry: P ) ) + if ContainerRegistry._ContainerRegistry__instance is not None and ContainerRegistry._ContainerRegistry__instance._db_connection is not None: + ContainerRegistry._ContainerRegistry__instance._db_connection.close() + ContainerRegistry._ContainerRegistry__instance._db_connection = None ContainerRegistry._ContainerRegistry__instance = None # Reset the private instance variable every time registry = ContainerRegistry(application) From bd5db050de4f09d0bfbf76ae2138fce151cdacf7 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 16:29:11 +0200 Subject: [PATCH 05/21] Fix unit test. Take the cache into account when testing the has-error function. part of CURA-11761 --- tests/Settings/TestContainerStack.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/Settings/TestContainerStack.py b/tests/Settings/TestContainerStack.py index bae367a51b..ed700f9d59 100644 --- a/tests/Settings/TestContainerStack.py +++ b/tests/Settings/TestContainerStack.py @@ -8,6 +8,7 @@ import pytest +from UM.Decorators import CachedMemberFunctions from UM.Settings.ContainerRegistry import ContainerRegistry from UM.Settings.ContainerStack import ContainerStack from UM.Settings.ContainerStack import IncorrectVersionError @@ -815,6 +816,9 @@ def test_getHasErrors(container_stack): # We won't get any wrong validation states, so it shouldn't have errors. assert not container_stack.hasErrors() + # Clear the cache (so 'hasErrors' recalculates, as from the perspective of the container-stack, it hasn't changed). + CachedMemberFunctions.clearInstanceCache(container_stack) + # Fake the property so it does return validation state container_stack.getProperty = MagicMock(return_value = ValidatorState.MaximumError) assert container_stack.hasErrors() # Now the container stack has errors! From 28522051e5aeecae40f613be9e4d127336c7e018 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 16:34:21 +0200 Subject: [PATCH 06/21] Fix cache filling/clearing errors caught by unit-tests. Code that is work in progress has some grody issues sometimes. I'm lucky this was caught by the unit-tests. Deserialize obviously changes the object, and morever shouldn't be cached itself! Also fix if somehow the instance deletion functionality is called twice somehow. (This happened at one point when fixing the tests -- not sure if that'll ever be an issue in the current tests or the actual code, but defensive programming is OK.) part of CURA-11761 --- UM/Decorators.py | 3 ++- UM/Settings/ContainerStack.py | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/UM/Decorators.py b/UM/Decorators.py index c5679f5e90..f25d1defea 100644 --- a/UM/Decorators.py +++ b/UM/Decorators.py @@ -156,7 +156,8 @@ def clearInstanceCache(cls, instance): @classmethod def deleteInstanceCache(cls, instance): - del cls.getInstance()._cache[instance] + if instance in cls.getInstance()._cache: + del cls.getInstance()._cache[instance] def __init__(self): if CachedMemberFunctions.__instance is not None: diff --git a/UM/Settings/ContainerStack.py b/UM/Settings/ContainerStack.py index f06603416c..417814ff92 100644 --- a/UM/Settings/ContainerStack.py +++ b/UM/Settings/ContainerStack.py @@ -409,7 +409,6 @@ def getVersionFromSerialized(cls, serialized: str) -> Optional[int]: Logger.log("d", "Could not get version from serialized: %s", e) return version - @cachePerInstance def deserialize(self, serialized: str, file_name: Optional[str] = None) -> str: """:copydoc ContainerInterface::deserialize @@ -418,6 +417,8 @@ def deserialize(self, serialized: str, file_name: Optional[str] = None) -> str: TODO: Expand documentation here, include the fact that this should _not_ include all containers """ + CachedMemberFunctions.clearInstanceCache(self) + # Update the serialized data first serialized = super().deserialize(serialized, file_name) parser = self._readAndValidateSerialized(serialized) @@ -839,6 +840,8 @@ def getErrorKeys(self) -> List[str]: # In addition, it allows us to emit a single signal that reports all properties that # have changed. def _collectPropertyChanges(self, key: str, property_name: str) -> None: + CachedMemberFunctions.clearInstanceCache(self) + if key not in self._property_changes: self._property_changes[key] = set() From fe3543acf3063e9909ab0c5cb06e010c670af1e2 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 17:13:49 +0200 Subject: [PATCH 07/21] Cache function-results from Settings-instances. core part of CURA-11761 --- UM/Settings/SettingInstance.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index 6f862d2b1a..0b4591d538 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -3,9 +3,11 @@ import copy #To implement deepcopy. import enum +from functools import lru_cache import os from typing import Any, cast, Dict, Iterable, List, Optional, Set, TYPE_CHECKING +from UM.Decorators import CachedMemberFunctions, cachePerInstance from UM.Settings.Interfaces import ContainerInterface from UM.Signal import Signal, signalemitter from UM.Logger import Logger @@ -92,6 +94,7 @@ def __init__(self, definition: SettingDefinition, container: ContainerInterface, self.__property_values = {} # type: Dict[str, Any] + @cachePerInstance def getPropertyNames(self) -> Iterable[str]: """Get a list of all supported property names""" @@ -129,9 +132,13 @@ def __eq__(self, other: object) -> bool: return False return True + def __hash__(self) -> int: + return id(self) + def __ne__(self, other: object) -> bool: return not (self == other) + @cachePerInstance def __getattr__(self, name: str) -> Any: if name == "_SettingInstance__property_values": # Prevent infinite recursion when __property_values is not set. @@ -157,6 +164,8 @@ def setProperty(self, name: str, value: Any, container: Optional[ContainerInterf Logger.log("e", "Tried to set property %s which is a read-only property", name) return + CachedMemberFunctions.clearInstanceCache(self) + if name not in self.__property_values or value != self.__property_values[name]: if isinstance(value, str) and value.strip().startswith("="): value = SettingFunction.SettingFunction(value[1:]) @@ -222,6 +231,7 @@ def state(self) -> InstanceState: return self._state def resetState(self) -> None: + CachedMemberFunctions.clearInstanceCache(self) self._state = InstanceState.Default def __repr__(self) -> str: @@ -235,6 +245,8 @@ def updateRelations(self, container: ContainerInterface, emit_signals: bool = Tr property_names.remove("value") # Move "value" to the front of the list so we always update that first. property_names.insert(0, "value") + CachedMemberFunctions.clearInstanceCache(self) + for property_name in property_names: if SettingDefinition.isReadOnlyProperty(property_name): continue @@ -252,6 +264,7 @@ def updateRelations(self, container: ContainerInterface, emit_signals: bool = Tr container.propertyChanged.emit(relation.target.key, "validationState") @staticmethod + # TODO: Passing sets and lists (which can't be hashed) prevents caching of this function. def _listRelations(key: str, relations_set: Set["SettingRelation"], relations: List["SettingRelation"], roles: List[str]) -> None: """Recursive function to put all settings that require eachother for changes of a property value in a list From cd1562fd4b0a91f1398abd121e4f9a057d1ecaa3 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 17:53:06 +0200 Subject: [PATCH 08/21] Cache function-results from instance containers. core part of CURA-11761 --- UM/Settings/InstanceContainer.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/UM/Settings/InstanceContainer.py b/UM/Settings/InstanceContainer.py index c59dfb898e..c608ccafe9 100644 --- a/UM/Settings/InstanceContainer.py +++ b/UM/Settings/InstanceContainer.py @@ -10,6 +10,7 @@ from PyQt6.QtCore import QObject, pyqtProperty, pyqtSignal from PyQt6.QtQml import QQmlEngine #To take ownership of this class ourselves. +from UM.Decorators import CachedMemberFunctions, cachePerInstance from UM.FastConfigParser import FastConfigParser from UM.Trust import Trust from UM.Decorators import override @@ -143,6 +144,7 @@ def __getstate__(self) -> Dict[str, Any]: def __setstate__(self, state: Dict[str, Any]) -> None: """For pickle support""" + CachedMemberFunctions.clearInstanceCache(self) self.__dict__.update(state) @classmethod @@ -178,6 +180,7 @@ def getId(self) -> str: def setCachedValues(self, cached_values: Dict[str, Any]) -> None: if not self._instances: + CachedMemberFunctions.clearInstanceCache(self) self._cached_values = cached_values else: Logger.log("w", "Unable set values to be lazy loaded when values are already loaded ") @@ -200,6 +203,7 @@ def setPath(self, path: str) -> None: Reimplemented from ContainerInterface """ + CachedMemberFunctions.clearInstanceCache(self) self._path = path def getName(self) -> str: @@ -212,6 +216,7 @@ def getName(self) -> str: def setName(self, name: str) -> None: if name != self.getName(): + CachedMemberFunctions.clearInstanceCache(self) self._metadata["name"] = name self._dirty = True self.nameChanged.emit() @@ -251,6 +256,7 @@ def setMetaData(self, metadata: Dict[str, Any]) -> None: if metadata == self._metadata: return #No need to do anything or even emit the signal. + CachedMemberFunctions.clearInstanceCache(self) #We'll fill a temporary dictionary with all the required metadata and overwrite it with the new metadata. #This way it is ensured that at least the required metadata is still there. self._metadata = { @@ -285,6 +291,7 @@ def setMetaDataEntry(self, key: str, value: Any) -> None: """ if key not in self._metadata or self._metadata[key] != value: + CachedMemberFunctions.clearInstanceCache(self) self._metadata[key] = value self._dirty = True self.metaDataChanged.emit(self) @@ -298,8 +305,10 @@ def setDirty(self, dirty: bool) -> None: if self._read_only: Logger.log("w", "Tried to set dirty on read-only object.") else: + CachedMemberFunctions.clearInstanceCache(self) self._dirty = dirty + @cachePerInstance def getProperty(self, key: str, property_name: str, context: PropertyEvaluationContext = None) -> Any: """:copydoc ContainerInterface::getProperty @@ -318,6 +327,7 @@ def getProperty(self, key: str, property_name: str, context: PropertyEvaluationC return None + @cachePerInstance def hasProperty(self, key: str, property_name: str) -> bool: """:copydoc ContainerInterface::hasProperty @@ -346,6 +356,8 @@ def _instantiateMissingSettingInstancesInCache(self) -> None: if not self._cached_values: return + CachedMemberFunctions.clearInstanceCache(self) + for key, value in self._cached_values.items(): if key not in self._instances: if not self.getDefinition(): @@ -387,6 +399,8 @@ def setProperty(self, key: str, property_name: str, property_value: Any, contain property_name, property_value, key, self.id)) return + CachedMemberFunctions.clearInstanceCache(self) + if key not in self._instances: try: definition = self.getDefinition() @@ -414,12 +428,14 @@ def setProperty(self, key: str, property_name: str, property_value: Any, contain def clear(self) -> None: """Remove all instances from this container.""" + CachedMemberFunctions.clearInstanceCache(self) self._instantiateCachedValues() all_keys = self._instances.copy() for key in all_keys: self.removeInstance(key, postpone_emit=True) self.sendPostponedEmits() + @cachePerInstance def getAllKeys(self) -> Set[str]: """Get all the keys of the instances of this container :returns: list of keys @@ -588,6 +604,8 @@ def deserialize(self, serialized: str, file_name: Optional[str] = None) -> str: Reimplemented from ContainerInterface """ + CachedMemberFunctions.clearInstanceCache(self) + # update the serialized data first serialized = super().deserialize(serialized, file_name) parser = self._readAndValidateSerialized(serialized) @@ -656,6 +674,7 @@ def _instantiateCachedValues(self) -> None: """Instance containers are lazy loaded. This function ensures that it happened.""" if not self._cached_values: return + CachedMemberFunctions.clearInstanceCache(self) definition = self.getDefinition() try: for key, value in self._cached_values.items(): @@ -665,6 +684,7 @@ def _instantiateCachedValues(self) -> None: self._cached_values = None + # TODO: Deal with cacheing kwargs type arguments, as that is passed as an 'unhashable' dict. def findInstances(self, **kwargs: Any) -> List[SettingInstance]: """Find instances matching certain criteria. @@ -682,6 +702,7 @@ def findInstances(self, **kwargs: Any) -> List[SettingInstance]: return result + @cachePerInstance def getInstance(self, key: str) -> Optional[SettingInstance]: """Get an instance by key""" @@ -699,6 +720,8 @@ def addInstance(self, instance: SettingInstance) -> None: if key in self._instances: return + CachedMemberFunctions.clearInstanceCache(self) + instance.propertyChanged.connect(self.propertyChanged) instance.propertyChanged.emit(key, "value") self._instances[key] = instance @@ -713,6 +736,8 @@ def removeInstance(self, key: str, postpone_emit: bool = False) -> None: if key not in self._instances: return + CachedMemberFunctions.clearInstanceCache(self) + instance = self._instances[key] del self._instances[key] if postpone_emit: @@ -739,6 +764,8 @@ def removeInstance(self, key: str, postpone_emit: bool = False) -> None: def update(self) -> None: """Update all instances from this container.""" + CachedMemberFunctions.clearInstanceCache(self) + self._instantiateCachedValues() for key, instance in self._instances.items(): instance.propertyChanged.emit(key, "value") @@ -749,6 +776,7 @@ def update(self) -> None: self.propertyChanged.emit(key, property_name) self._dirty = True + @cachePerInstance def getDefinition(self) -> DefinitionContainerInterface: """Get the DefinitionContainer used for new instance creation.""" @@ -766,6 +794,7 @@ def setDefinition(self, definition_id: str) -> None: way of figuring out what SettingDefinition to use when creating a new SettingInstance. """ + CachedMemberFunctions.clearInstanceCache(self) self._metadata["definition"] = definition_id self._definition = None @@ -798,6 +827,7 @@ def sendPostponedEmits(self) -> None: signal, signal_arg = self._postponed_emits.pop(0) signal.emit(*signal_arg) + @cachePerInstance def getAllKeysWithUserState(self)-> Set[str]: """Get the keys of all the setting having a User state""" self._instantiateCachedValues() From 7527d0d16dba317b846631e6ba95007ef9f84196 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 28 Aug 2024 17:54:50 +0200 Subject: [PATCH 09/21] Small update documentation. done as part of CURA-11761 --- UM/Settings/ContainerStack.py | 1 + UM/Settings/InstanceContainer.py | 2 +- UM/Settings/SettingInstance.py | 2 +- tests/Settings/TestContainerStack.py | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/UM/Settings/ContainerStack.py b/UM/Settings/ContainerStack.py index 417814ff92..e5c4613732 100644 --- a/UM/Settings/ContainerStack.py +++ b/UM/Settings/ContainerStack.py @@ -194,6 +194,7 @@ def _getRawMetaDataEntry(self, entry: str) -> Any: return value + # Note that '_getRawMetaDataEntry' is cached, not this one, because default may be a list and that's unhashable. def getMetaDataEntry(self, entry: str, default: Any = None) -> Any: """:copydoc ContainerInterface::getMetaDataEntry diff --git a/UM/Settings/InstanceContainer.py b/UM/Settings/InstanceContainer.py index c608ccafe9..c0a7f0b877 100644 --- a/UM/Settings/InstanceContainer.py +++ b/UM/Settings/InstanceContainer.py @@ -1,4 +1,4 @@ -# Copyright (c) 2022 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. import configparser diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index 0b4591d538..307cf8b69d 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -1,4 +1,4 @@ -# Copyright (c) 2018 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. import copy #To implement deepcopy. diff --git a/tests/Settings/TestContainerStack.py b/tests/Settings/TestContainerStack.py index ed700f9d59..c9178ce541 100644 --- a/tests/Settings/TestContainerStack.py +++ b/tests/Settings/TestContainerStack.py @@ -1,4 +1,4 @@ -# Copyright (c) 2018 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. from typing import Optional From 69a60b8761df9c157119d061d1c5713db759d67a Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 3 Sep 2024 11:39:20 +0200 Subject: [PATCH 10/21] Workaround for sets not hashing (use frozenset) and cache definition. Sets can normally not be hashed, which means we can't use lru_cache or use our own class-cache mechanism that derives from that for methods that have (normal) sets, lists or dicts as argument. Solve that by making some of these methods arguments, the results of which we want to be cached, into frozen sets instead. For this reason, it was convenient to have a method in the SettingDefinition file return a fronzen set as well. The result of which could be cached as well, and then why not do the entire file, since we already need to import the caching mechanisms, and clear the cache on the appropriate moments anyway. part of CURA-11761 --- UM/Settings/SettingDefinition.py | 53 +++++++++++++++++++++++++++++++- UM/Settings/SettingInstance.py | 16 +++++----- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/UM/Settings/SettingDefinition.py b/UM/Settings/SettingDefinition.py index c6f9962424..ad2d568509 100644 --- a/UM/Settings/SettingDefinition.py +++ b/UM/Settings/SettingDefinition.py @@ -1,13 +1,15 @@ -# Copyright (c) 2019 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. import ast +from functools import lru_cache import json import enum import collections import re from typing import Any, List, Dict, Callable, Match, Set, Union, Optional +from UM.Decorators import CachedMemberFunctions, cachePerInstance from UM.Logger import Logger from UM.Settings.Interfaces import DefinitionContainerInterface from UM.i18n import i18nCatalog @@ -116,6 +118,8 @@ def __init__(self, key: str, container: Optional[DefinitionContainerInterface] = self.__property_values = {} # type: Dict[str, Any] + self.__init_done = True + def extend_category(self, value_id: str, value_display: str, plugin_id: Optional[str] = None, plugin_version: Optional[str] = None) -> None: """Append a category to the setting. @@ -129,6 +133,7 @@ def extend_category(self, value_id: str, value_display: str, plugin_id: Optional value_id = f"PLUGIN::{plugin_id}@{plugin_version}::{value_id}" elif plugin_id is not None or plugin_version is not None: raise ValueError("Both plugin_id and plugin_version must be provided if one of them is provided.") + CachedMemberFunctions.clearInstanceCache(self) self.options[value_id] = value_display def __getattr__(self, name: str) -> Any: @@ -148,6 +153,9 @@ def __setattr__(self, name: str, value: Any) -> None: if name in self.__property_definitions: raise NotImplementedError("Setting of property {0} not supported".format(name)) + if "__init_done" in self.__dict__: + CachedMemberFunctions.clearInstanceCache(self) + super().__setattr__(name, value) def __hash__(self): @@ -171,6 +179,9 @@ def __setstate__(self, state): behaviour doesn't combine well with a non-default __getattr__. """ + if "__init_done" in self.__dict__: + CachedMemberFunctions.clearInstanceCache(self) + self.__dict__.update(state) # For 4.0 we added the _all_keys property, but the pickling fails to restore this. # This is just there to prevent issues for developers, since only releases ignore caches. @@ -223,6 +234,15 @@ def relations(self) -> List["SettingRelation"]: return self._relations + @cachePerInstance + def relationsAsFrozenSet(self) -> frozenset["SettingRelation"]: + """A frozen set of SettingRelation objects of this setting. + + :return: :type{frozenset} + """ + + return frozenset(self.relations) + def serialize(self) -> str: """Serialize this setting to a string. @@ -231,6 +251,7 @@ def serialize(self) -> str: pass + @cachePerInstance def getAllKeys(self) -> Set[str]: """Gets the key of this setting definition and of all its descendants. @@ -269,12 +290,14 @@ def deserialize(self, serialized: Union[str, Dict[str, Any]]) -> None: :param serialized: :type{string or dict} A serialized representation of this setting. """ + CachedMemberFunctions.clearInstanceCache(self) if isinstance(serialized, dict): self._deserialize_dict(serialized) else: parsed = json.loads(serialized, object_pairs_hook=collections.OrderedDict) self._deserialize_dict(parsed) + @cachePerInstance def getChild(self, key: str) -> Optional["SettingDefinition"]: """Get a child by key @@ -296,6 +319,7 @@ def getChild(self, key: str) -> Optional["SettingDefinition"]: return None + @cachePerInstance def _matches1l8nProperty(self, property_name: str, value: Any, catalog) -> bool: try: property_value = getattr(self, property_name) @@ -418,6 +442,7 @@ def findDefinitions(self, **kwargs: Any) -> List["SettingDefinition"]: return definitions + @cachePerInstance def isAncestor(self, key: str) -> bool: """Check whether a certain setting is an ancestor of this definition. @@ -431,6 +456,7 @@ def isAncestor(self, key: str) -> bool: return key in self.__ancestors + @cachePerInstance def isDescendant(self, key: str) -> bool: """Check whether a certain setting is a descendant of this definition. @@ -444,6 +470,7 @@ def isDescendant(self, key: str) -> bool: return key in self.__descendants + @cachePerInstance def getAncestors(self) -> Set[str]: """Get a set of keys representing the setting's ancestors.""" @@ -490,10 +517,12 @@ def addSupportedProperty(cls, name: str, property_type: DefinitionPropertyType, value should be re-evaluated. """ + cls._clearClassCache() cls.__property_definitions[name] = {"type": property_type, "required": required, "read_only": read_only, "default": default, "depends_on": depends_on} @classmethod + @lru_cache def getPropertyNames(cls, def_type: DefinitionPropertyType = None) -> List[str]: """Get the names of all supported properties. @@ -507,6 +536,7 @@ def getPropertyNames(cls, def_type: DefinitionPropertyType = None) -> List[str]: return [key for key, value in cls.__property_definitions.items() if not def_type or value["type"] == def_type] @classmethod + @lru_cache def hasProperty(cls, name: str) -> bool: """Check if a property with the specified name is defined as a supported property. @@ -518,6 +548,7 @@ def hasProperty(cls, name: str) -> bool: return name in cls.__property_definitions @classmethod + @lru_cache def getPropertyType(cls, name: str) -> Optional[str]: """Get the type of a specified property. @@ -532,6 +563,7 @@ def getPropertyType(cls, name: str) -> Optional[str]: return None @classmethod + @lru_cache def isRequiredProperty(cls, name: str) -> bool: """Check if the specified property is considered a required property. @@ -549,6 +581,7 @@ def isRequiredProperty(cls, name: str) -> bool: return False @classmethod + @lru_cache def isReadOnlyProperty(cls, name: str) -> bool: """Check if the specified property is considered a read-only property. @@ -565,6 +598,7 @@ def isReadOnlyProperty(cls, name: str) -> bool: return False @classmethod + @lru_cache def dependsOnProperty(cls, name: str) -> Optional[str]: """Check if the specified property depends on another property @@ -590,9 +624,11 @@ def addSettingType(cls, type_name: str, from_string: Optional[Callable[[str], An """ + cls._clearClassCache() cls.__type_definitions[type_name] = { "from": from_string, "to": to_string, "validator": validator } @classmethod + @lru_cache def settingValueFromString(cls, type_name: str, string_value: str) -> Any: """Convert a string to a value according to a setting type. @@ -639,6 +675,7 @@ def settingValueToString(cls, type_name: str, value: Any) -> str: return value @classmethod + @lru_cache def getValidatorForType(cls, type_name: str) -> Callable[[str],Validator]: """Get the validator type for a certain setting type.""" @@ -653,6 +690,7 @@ def _deserialize_dict(self, serialized: Dict[str, Any]) -> None: Deserialize from a dictionary """ + CachedMemberFunctions.clearInstanceCache(self) self._children = [] self._relations = [] @@ -695,6 +733,7 @@ def _deserialize_dict(self, serialized: Dict[str, Any]) -> None: self.__ancestors = self._updateAncestors() self.__descendants = self._updateDescendants() + @cachePerInstance def _updateAncestors(self) -> Set[str]: result = set() # type: Set[str] @@ -706,6 +745,7 @@ def _updateAncestors(self) -> Set[str]: return result def _updateDescendants(self, definition: "SettingDefinition" = None) -> Dict[str, "SettingDefinition"]: + CachedMemberFunctions.clearInstanceCache(self) result = {} self._all_keys = set() # Reset the keys cache. if not definition: @@ -717,6 +757,17 @@ def _updateDescendants(self, definition: "SettingDefinition" = None) -> Dict[str return result + @classmethod + def _clearClassCache(cls): + cls.getPropertyNames.cache_clear() + cls.hasProperty.cache_clear() + cls.getPropertyType.cache_clear() + cls.isRequiredProperty.cache_clear() + cls.isReadOnlyProperty.cache_clear() + cls.dependsOnProperty.cache_clear() + cls.settingValueFromString.cache_clear() + cls.getValidatorForType.cache_clear() + __property_definitions = { # The name of the setting. Only used for display purposes. "label": {"type": DefinitionPropertyType.TranslatedString, "required": True, "read_only": True, "default": "", "depends_on" : None}, diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index 307cf8b69d..8493c4761d 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -38,8 +38,8 @@ def _traceRelations(instance: "SettingInstance", container: ContainerInterface, if SettingDefinition.isReadOnlyProperty(property_name): continue - changed_relations: Set[SettingRelation] = set() - SettingInstance._listRelations(instance.definition.key, changed_relations, instance.definition.relations, [property_name]) + changed_relations: frozenset[SettingRelation] = frozenset() + SettingInstance._listRelations(instance.definition.key, changed_relations, instance.definition.relationsAsFrozenSet(), frozenset({property_name})) for relation in changed_relations: Logger.log("d", "Emitting property change for relation {0}", relation) @@ -254,8 +254,8 @@ def updateRelations(self, container: ContainerInterface, emit_signals: bool = Tr # TODO: We should send this as a single change event instead of several of them. # That would increase performance by reducing the amount of updates. if emit_signals: - changed_relations: Set[SettingRelation] = set() - SettingInstance._listRelations(self.definition.key, changed_relations, self._definition.relations, [property_name]) + changed_relations: frozenset[SettingRelation] = frozenset() + SettingInstance._listRelations(self.definition.key, changed_relations, self._definition.relationsAsFrozenSet(), frozenset([property_name])) for relation in changed_relations: container.propertyChanged.emit(relation.target.key, relation.role) @@ -264,8 +264,8 @@ def updateRelations(self, container: ContainerInterface, emit_signals: bool = Tr container.propertyChanged.emit(relation.target.key, "validationState") @staticmethod - # TODO: Passing sets and lists (which can't be hashed) prevents caching of this function. - def _listRelations(key: str, relations_set: Set["SettingRelation"], relations: List["SettingRelation"], roles: List[str]) -> None: + @lru_cache + def _listRelations(key: str, relations_set: frozenset["SettingRelation"], relations: frozenset["SettingRelation"], roles: frozenset[str]) -> None: """Recursive function to put all settings that require eachother for changes of a property value in a list :param relations_set: :type{set} Set of keys (strings) of settings that are influenced @@ -289,10 +289,10 @@ def _listRelations(key: str, relations_set: Set["SettingRelation"], relations: L if relation in relations_set: continue - relations_set.add(relation) + relations_set = relations_set.union({relation}) property_names = SettingDefinition.getPropertyNames() property_names.remove("value") # Move "value" to the front of the list so we always update that first. property_names.insert(0, "value") - SettingInstance._listRelations(key, relations_set, relation.target.relations, property_names) + SettingInstance._listRelations(key, relations_set, relation.target.relationsAsFrozenSet(), frozenset(property_names)) From 610854ba585e90b7c96607e8c15e3654f9c3962a Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 3 Sep 2024 11:43:30 +0200 Subject: [PATCH 11/21] Cache results for _getPropertyValue. Not any other methods, since they're either pyqtproperties (or setters, etc.) and that doesn't seem to play too well with the cacheing mechanism. The properties use this internally, so it's enough anyway. CURA-11761 --- UM/Settings/Models/SettingPropertyProvider.py | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/UM/Settings/Models/SettingPropertyProvider.py b/UM/Settings/Models/SettingPropertyProvider.py index bff05c463d..86e14037db 100644 --- a/UM/Settings/Models/SettingPropertyProvider.py +++ b/UM/Settings/Models/SettingPropertyProvider.py @@ -1,4 +1,4 @@ -# Copyright (c) 2022 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. from typing import Optional, List, Set, Any, OrderedDict @@ -7,6 +7,7 @@ from PyQt6.QtQml import QQmlPropertyMap from UM.FlameProfiler import pyqtSlot +from UM.Decorators import CachedMemberFunctions, cachePerInstance from UM.Logger import Logger from UM.Application import Application from UM.Settings.ContainerStack import ContainerStack @@ -57,6 +58,8 @@ def setContainerStack(self, stack: Optional[ContainerStack]) -> None: if self._stack == stack: return # Nothing to do, attempting to set stack to the same value. + CachedMemberFunctions.clearInstanceCache(self) + if self._stack: self._stack.propertiesChanged.disconnect(self._onPropertiesChanged) self._stack.containersChanged.disconnect(self._containersChanged) @@ -77,6 +80,8 @@ def setContainerStackId(self, stack_id: str) -> None: if stack_id == self.containerStackId: return # No change. + CachedMemberFunctions.clearInstanceCache(self) + if stack_id: if stack_id == "global": self.setContainerStack(Application.getInstance().getGlobalContainerStack()) @@ -109,6 +114,7 @@ def containerStack(self) -> Optional[ContainerInterface]: def setRemoveUnusedValue(self, remove_unused_value: bool) -> None: if self._remove_unused_value != remove_unused_value: + CachedMemberFunctions.clearInstanceCache(self) self._remove_unused_value = remove_unused_value self.removeUnusedValueChanged.emit() @@ -120,6 +126,7 @@ def setWatchedProperties(self, properties: List[str]) -> None: """Set the watchedProperties property.""" if properties != self._watched_properties: + CachedMemberFunctions.clearInstanceCache(self) self._watched_properties = properties self._updateDelayed() self.watchedPropertiesChanged.emit() @@ -137,6 +144,7 @@ def setKey(self, key: str) -> None: """Set the key property.""" if key != self._key: + CachedMemberFunctions.clearInstanceCache(self) self._key = key self._validator = None self._updateDelayed() @@ -162,6 +170,7 @@ def forcePropertiesChanged(self): def setStoreIndex(self, index): if index != self._store_index: + CachedMemberFunctions.clearInstanceCache(self) self._store_index = index self.storeIndexChanged.emit() @@ -200,6 +209,8 @@ def setPropertyValue(self, property_name, property_value): if isinstance(container, DefinitionContainer): return + CachedMemberFunctions.clearInstanceCache(self) + # In some cases we clean some stuff and the result is as when nothing as been changed manually. if property_name == "value" and self._remove_unused_value: for index in self._stack_levels: @@ -268,6 +279,7 @@ def getPropertyValueAsString(self, property_name: str) -> str: @pyqtSlot(int) def removeFromContainer(self, index: int) -> None: + CachedMemberFunctions.clearInstanceCache(self) current_stack = self._stack while current_stack: num_containers = len(current_stack.getContainers()) @@ -321,6 +333,8 @@ def isValueUsed(self) -> bool: return self._value_used def _onPropertiesChanged(self, key: str, property_names: List[str]) -> None: + CachedMemberFunctions.clearInstanceCache(self) + if key != self._key: if key in self._relations: self._value_used = None @@ -364,6 +378,8 @@ def _update(self, container = None): if not self._stack or not self._watched_properties or not self._key: return + CachedMemberFunctions.clearInstanceCache(self) + self._updateStackLevels() relations = self._stack.getProperty(self._key, "relations") if relations: # If the setting doesn't have the property relations, None is returned @@ -397,6 +413,7 @@ def _storeIndexChanged(self): def _updateStackLevels(self) -> None: """Updates the self._stack_levels field, which indicates at which levels in the stack the property is set.""" + CachedMemberFunctions.clearInstanceCache(self) levels = [] # Start looking at the stack this provider is attached to. current_stack = self._stack @@ -416,6 +433,7 @@ def _updateStackLevels(self) -> None: self._stack_levels = levels self.stackLevelChanged.emit() + @cachePerInstance def _getPropertyValue(self, property_name): # Use the evaluation context to skip certain containers context = PropertyEvaluationContext(self._stack) From 4afc270ac3d770b95fdf67a349f35122abdc04d3 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 3 Sep 2024 16:07:16 +0200 Subject: [PATCH 12/21] Refactor instance-methods-cache class. - accept, but do not attempt to cache, keyword-arguments (kwargs) - make it so that the class doesn't depend on an instance (put the cache directly as a class-variable) part of CURA-11761 --- UM/Decorators.py | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/UM/Decorators.py b/UM/Decorators.py index f25d1defea..8ea3759f19 100644 --- a/UM/Decorators.py +++ b/UM/Decorators.py @@ -142,39 +142,31 @@ def timed(*args, **kw): class CachedMemberFunctions: - __instance = None - - @classmethod - def getInstance(cls): - if cls.__instance is None: - cls.__instance = cls() - return cls.__instance + __cache = {} @classmethod def clearInstanceCache(cls, instance): - cls.getInstance()._cache[instance] = {} + cls.__cache[instance] = {} @classmethod def deleteInstanceCache(cls, instance): - if instance in cls.getInstance()._cache: - del cls.getInstance()._cache[instance] + if instance in cls.__cache: + del cls.__cache[instance] - def __init__(self): - if CachedMemberFunctions.__instance is not None: - raise RuntimeError(f"Attempt to instantiate singleton '{self.__class__.__name__}' more than once.") - self._cache = {} - - def callFunction(self, instance, function, *args, **kwargs): - # FIXME: Keyword arguments ('kwargs') will probably not _actually_ work here, since it's given as a dictionary, - # and the arguments need to be hashable in order for it to properly fit into the lru_cache. - if instance not in self._cache: - self._cache[instance] = {} - if function not in self._cache[instance]: - self._cache[instance][function] = lru_cache()(partial(function, instance)) - return self._cache[instance][function](*args, **kwargs) + @classmethod + def callMemberFunction(cls, instance, function, *args, **kwargs): + if kwargs is not None and len(kwargs) > 0: + # NOTE The `lru_cache` can't handle keyword-arguments (because it's a dict). + # We could make a frozendict, but that's probably a lot more hassle than it's worth, so just call normally. + return function(instance, *args, **kwargs) + if instance not in cls.__cache: + cls.__cache[instance] = {} + if function not in cls.__cache[instance]: + cls.__cache[instance][function] = lru_cache()(partial(function, instance)) + return cls.__cache[instance][function](*args) def cachePerInstance(function): def wrapper(instance, *args, **kwargs): - return CachedMemberFunctions.getInstance().callFunction(instance, function, *args, **kwargs) + return CachedMemberFunctions.callMemberFunction(instance, function, *args, **kwargs) return wrapper From 032de4b10f012658308869c6055f1e0b257f057e Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 3 Sep 2024 16:08:32 +0200 Subject: [PATCH 13/21] Clean up cache (more) when objects get deleted. part of CURA-11761 --- UM/Settings/ContainerStack.py | 2 +- UM/Settings/InstanceContainer.py | 4 ++++ UM/Settings/Models/SettingPropertyProvider.py | 4 ++++ UM/Settings/SettingDefinition.py | 4 ++++ UM/Settings/SettingInstance.py | 4 ++++ 5 files changed, 17 insertions(+), 1 deletion(-) diff --git a/UM/Settings/ContainerStack.py b/UM/Settings/ContainerStack.py index e5c4613732..c6a5f544c2 100644 --- a/UM/Settings/ContainerStack.py +++ b/UM/Settings/ContainerStack.py @@ -872,7 +872,7 @@ def __repr__(self) -> str: return str(self) def __del__(self) -> None: - # None of the 'parents' seem to have __dell__, so OK not to call `super.del` here. + # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. CachedMemberFunctions.deleteInstanceCache(self) diff --git a/UM/Settings/InstanceContainer.py b/UM/Settings/InstanceContainer.py index c0a7f0b877..f1b4fff796 100644 --- a/UM/Settings/InstanceContainer.py +++ b/UM/Settings/InstanceContainer.py @@ -816,6 +816,10 @@ def __str__(self) -> str: def __repr__(self) -> str: return str(self) + def __del__(self) -> None: + # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. + CachedMemberFunctions.deleteInstanceCache(self) + def sendPostponedEmits(self) -> None: """Send the postponed emits diff --git a/UM/Settings/Models/SettingPropertyProvider.py b/UM/Settings/Models/SettingPropertyProvider.py index 86e14037db..456c0bd849 100644 --- a/UM/Settings/Models/SettingPropertyProvider.py +++ b/UM/Settings/Models/SettingPropertyProvider.py @@ -470,3 +470,7 @@ def _getPropertyValue(self, property_name): return options_map return str(property_value) + + def __del__(self) -> None: + # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. + CachedMemberFunctions.deleteInstanceCache(self) diff --git a/UM/Settings/SettingDefinition.py b/UM/Settings/SettingDefinition.py index ad2d568509..f76c286a70 100644 --- a/UM/Settings/SettingDefinition.py +++ b/UM/Settings/SettingDefinition.py @@ -189,6 +189,10 @@ def __setstate__(self, state): if not hasattr(self, "_all_keys"): self._all_keys = set() + def __del__(self) -> None: + # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. + CachedMemberFunctions.deleteInstanceCache(self) + @property def key(self) -> str: """The key of this setting. diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index 8493c4761d..1936aa5a05 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -157,6 +157,10 @@ def __getattr__(self, name: str) -> Any: raise AttributeError("'SettingInstance' object has no attribute '{0}'".format(name)) + def __del__(self) -> None: + # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. + CachedMemberFunctions.deleteInstanceCache(self) + @call_if_enabled(_traceSetProperty, _isTraceEnabled()) def setProperty(self, name: str, value: Any, container: Optional[ContainerInterface] = None, emit_signals: bool = True) -> None: if SettingDefinition.hasProperty(name): From 0b6eb8b1f2290464f7e3636384a6ddd3a265c84d Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 3 Sep 2024 18:14:30 +0200 Subject: [PATCH 14/21] Class-member-cache: Add tests and documentation. part of CURA-11761 --- UM/Decorators.py | 5 +++++ tests/TestDecorators.py | 49 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/UM/Decorators.py b/UM/Decorators.py index 8ea3759f19..a3897d3c83 100644 --- a/UM/Decorators.py +++ b/UM/Decorators.py @@ -142,19 +142,24 @@ def timed(*args, **kw): class CachedMemberFunctions: + """Helper class to handle instance-cache w.r.t. results of member-functions decorated with '@cachePerInstance'.""" + __cache = {} @classmethod def clearInstanceCache(cls, instance): + """Clear all the cache-entries for the specified instance.""" cls.__cache[instance] = {} @classmethod def deleteInstanceCache(cls, instance): + """Completely delete the entry of the specified instance.""" if instance in cls.__cache: del cls.__cache[instance] @classmethod def callMemberFunction(cls, instance, function, *args, **kwargs): + """Call the specified member function, make use of (results) cache if available, and create if not.""" if kwargs is not None and len(kwargs) > 0: # NOTE The `lru_cache` can't handle keyword-arguments (because it's a dict). # We could make a frozendict, but that's probably a lot more hassle than it's worth, so just call normally. diff --git a/tests/TestDecorators.py b/tests/TestDecorators.py index 1f6c496ac4..cef2a3372a 100644 --- a/tests/TestDecorators.py +++ b/tests/TestDecorators.py @@ -1,9 +1,10 @@ -# Copyright (c) 2019 Ultimaker B.V. +# Copyright (c) 2024 UltiMaker # Uranium is released under the terms of the LGPLv3 or higher. +from unittest.mock import MagicMock import pytest -from UM.Decorators import interface +from UM.Decorators import interface, CachedMemberFunctions, cachePerInstance def test_interface(): def declare_interface(): @@ -99,3 +100,47 @@ class TestSubClass(TestInterface): sub = should_ignore_private_functions() assert sub is not None + + +def test_cachePerInstance(): + + bigDeal = MagicMock() + + class SomeClass: + def __init__(self): + self._map = {} + + @cachePerInstance + def getThing(self, a): + bigDeal() + return self._map.get(a, None) + + def setThing(self, a, b): + CachedMemberFunctions.clearInstanceCache(self) + self._map[a] = b + + instance = SomeClass() + + instance.setThing("marco", "polo") + instance.getThing("marco") + instance.getThing("marco") + assert instance.getThing("marco") == "polo" + assert bigDeal.call_count == 1 + + instance.setThing("marco", "bolo") + assert instance.getThing("marco") == "bolo" + assert bigDeal.call_count == 2 + instance.getThing("marco") + instance.getThing("marco") + assert bigDeal.call_count == 2 + + other = SomeClass() + other.setThing("marco", "yolo") + other.getThing("marco") + other.getThing("marco") + assert other.getThing("marco") == "yolo" + assert bigDeal.call_count == 3 + + assert len(CachedMemberFunctions._CachedMemberFunctions__cache) == 2 + CachedMemberFunctions.deleteInstanceCache(instance) + assert len(CachedMemberFunctions._CachedMemberFunctions__cache) == 1 From 5106ab30d151eaed79dacf3cf049f24130910c43 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 3 Sep 2024 18:37:09 +0200 Subject: [PATCH 15/21] Remove cache before cache unit-test begins. done as part of CURA-11761 --- tests/TestDecorators.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/TestDecorators.py b/tests/TestDecorators.py index cef2a3372a..bd67e7fe04 100644 --- a/tests/TestDecorators.py +++ b/tests/TestDecorators.py @@ -104,6 +104,7 @@ class TestSubClass(TestInterface): def test_cachePerInstance(): + CachedMemberFunctions._CachedMemberFunctions__cache = {} bigDeal = MagicMock() class SomeClass: From e8fb0c0d30ba9e75434b1d15cd0e3cbf94c7c6c1 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 10 Sep 2024 12:48:23 +0200 Subject: [PATCH 16/21] Also do a 'clear-instance' in the other branch. part of CURA-11761 --- UM/Settings/SettingInstance.py | 1 + 1 file changed, 1 insertion(+) diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index 1936aa5a05..1fd8b609e0 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -198,6 +198,7 @@ def setProperty(self, name: str, value: Any, container: Optional[ContainerInterf else: if name == "state": if value == "InstanceState.Calculated": + CachedMemberFunctions.clearInstanceCache(self) if self._state != InstanceState.Calculated: self._state = InstanceState.Calculated if emit_signals: From dae5d6c199b4ea3737da0543693abca2dcf76e88 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Tue, 10 Sep 2024 12:50:36 +0200 Subject: [PATCH 17/21] Accumulate by return value instead, since a frozen-set can't grow. Changed relations where coming up empty, no relations where handled properly, due to a change to frozen-sets. part of CURA-11761 --- UM/Settings/SettingInstance.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index 1fd8b609e0..7722b15de3 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -38,9 +38,7 @@ def _traceRelations(instance: "SettingInstance", container: ContainerInterface, if SettingDefinition.isReadOnlyProperty(property_name): continue - changed_relations: frozenset[SettingRelation] = frozenset() - SettingInstance._listRelations(instance.definition.key, changed_relations, instance.definition.relationsAsFrozenSet(), frozenset({property_name})) - + changed_relations = SettingInstance._listRelations(instance.definition.key, frozenset(), instance.definition.relationsAsFrozenSet(), frozenset({property_name})) for relation in changed_relations: Logger.log("d", "Emitting property change for relation {0}", relation) #container.propertyChanged.emit(relation.target.key, relation.role) @@ -259,8 +257,7 @@ def updateRelations(self, container: ContainerInterface, emit_signals: bool = Tr # TODO: We should send this as a single change event instead of several of them. # That would increase performance by reducing the amount of updates. if emit_signals: - changed_relations: frozenset[SettingRelation] = frozenset() - SettingInstance._listRelations(self.definition.key, changed_relations, self._definition.relationsAsFrozenSet(), frozenset([property_name])) + changed_relations = SettingInstance._listRelations(self.definition.key, frozenset(), self._definition.relationsAsFrozenSet(), frozenset([property_name])) for relation in changed_relations: container.propertyChanged.emit(relation.target.key, relation.role) @@ -270,7 +267,7 @@ def updateRelations(self, container: ContainerInterface, emit_signals: bool = Tr @staticmethod @lru_cache - def _listRelations(key: str, relations_set: frozenset["SettingRelation"], relations: frozenset["SettingRelation"], roles: frozenset[str]) -> None: + def _listRelations(key: str, relations_set: frozenset["SettingRelation"], relations: frozenset["SettingRelation"], roles: frozenset[str]) -> frozenset["SettingRelation"]: """Recursive function to put all settings that require eachother for changes of a property value in a list :param relations_set: :type{set} Set of keys (strings) of settings that are influenced @@ -300,4 +297,6 @@ def _listRelations(key: str, relations_set: frozenset["SettingRelation"], relati property_names.remove("value") # Move "value" to the front of the list so we always update that first. property_names.insert(0, "value") - SettingInstance._listRelations(key, relations_set, relation.target.relationsAsFrozenSet(), frozenset(property_names)) + relations_set = SettingInstance._listRelations(key, relations_set, relation.target.relationsAsFrozenSet(), frozenset(property_names)) + + return relations_set From eb698dc19c7902e537bf0662240058e237b72781 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 11 Sep 2024 11:55:38 +0200 Subject: [PATCH 18/21] Copy cache results in some instances and rename to lowercase. Copy (shallow) to prevent lists from being altered, because those are references in the cache. part of CURA-11761 --- UM/Decorators.py | 13 +++++++++-- UM/Settings/ContainerStack.py | 22 +++++++++---------- UM/Settings/InstanceContainer.py | 14 ++++++------ UM/Settings/Models/SettingPropertyProvider.py | 4 ++-- UM/Settings/SettingDefinition.py | 18 +++++++-------- UM/Settings/SettingInstance.py | 6 ++--- tests/TestDecorators.py | 14 ++++++++++-- 7 files changed, 55 insertions(+), 36 deletions(-) diff --git a/UM/Decorators.py b/UM/Decorators.py index a3897d3c83..344513faa9 100644 --- a/UM/Decorators.py +++ b/UM/Decorators.py @@ -142,7 +142,7 @@ def timed(*args, **kw): class CachedMemberFunctions: - """Helper class to handle instance-cache w.r.t. results of member-functions decorated with '@cachePerInstance'.""" + """Helper class to handle instance-cache w.r.t. results of member-functions decorated with '@cache_per_instance'.""" __cache = {} @@ -171,7 +171,16 @@ def callMemberFunction(cls, instance, function, *args, **kwargs): return cls.__cache[instance][function](*args) -def cachePerInstance(function): +def cache_per_instance(function): def wrapper(instance, *args, **kwargs): return CachedMemberFunctions.callMemberFunction(instance, function, *args, **kwargs) return wrapper + + +def cache_per_instance_copy_result(function): + def wrapper(instance, *args, **kwargs): + result = CachedMemberFunctions.callMemberFunction(instance, function, *args, **kwargs) + if hasattr(result, "copy"): + return result.copy() + return copy.copy(result) + return wrapper diff --git a/UM/Settings/ContainerStack.py b/UM/Settings/ContainerStack.py index c6a5f544c2..8547cdbe26 100644 --- a/UM/Settings/ContainerStack.py +++ b/UM/Settings/ContainerStack.py @@ -13,7 +13,7 @@ import UM.FlameProfiler from UM.ConfigurationErrorMessage import ConfigurationErrorMessage -from UM.Decorators import CachedMemberFunctions, cachePerInstance +from UM.Decorators import CachedMemberFunctions, cache_per_instance, cache_per_instance_copy_result from UM.Signal import Signal, signalemitter from UM.PluginObject import PluginObject from UM.MimeTypeDatabase import MimeTypeDatabase, MimeType @@ -182,7 +182,7 @@ def setMetaData(self, meta_data: Dict[str, Any]) -> None: metaDataChanged = pyqtSignal(QObject) metaData = pyqtProperty("QVariantMap", fget = getMetaData, fset = setMetaData, notify = metaDataChanged) - @cachePerInstance + @cache_per_instance def _getRawMetaDataEntry(self, entry: str) -> Any: value = self._metadata.get(entry, None) @@ -229,7 +229,7 @@ def setDirty(self, dirty: bool) -> None: containersChanged = Signal() - @cachePerInstance + @cache_per_instance def getProperty(self, key: str, property_name: str, context: Optional[PropertyEvaluationContext] = None) -> Any: """:copydoc ContainerInterface::getProperty @@ -256,7 +256,7 @@ def getProperty(self, key: str, property_name: str, context: Optional[PropertyEv return value - @cachePerInstance + @cache_per_instance def getRawProperty(self, key: str, property_name: str, *, context: Optional[PropertyEvaluationContext] = None, use_next: bool = True, skip_until_container: Optional[ContainerInterface] = None) -> Any: """Retrieve a property of a setting by key and property name. @@ -306,7 +306,7 @@ def getRawProperty(self, key: str, property_name: str, *, context: Optional[Prop else: return None - @cachePerInstance + @cache_per_instance def hasProperty(self, key: str, property_name: str) -> bool: """:copydoc ContainerInterface::hasProperty @@ -509,7 +509,7 @@ def deserializeMetadata(cls, serialized: str, container_id: str) -> List[Dict[st return [metadata] - @cachePerInstance + @cache_per_instance_copy_result def getAllKeys(self) -> Set[str]: """Get all keys known to this container stack. @@ -527,7 +527,7 @@ def getAllKeys(self) -> Set[str]: keys |= self._next_stack.getAllKeys() return keys - @cachePerInstance + @cache_per_instance_copy_result def getAllKeysWithUserState(self) -> Set[str]: """Get a subset of all the settings keys in all the containers having a User state @@ -545,7 +545,7 @@ def getAllKeysWithUserState(self) -> Set[str]: return settings - @cachePerInstance + @cache_per_instance def getContainers(self) -> List[ContainerInterface]: """Get a list of all containers in this stack. @@ -616,7 +616,7 @@ def setPath(self, path: str) -> None: CachedMemberFunctions.clearInstanceCache(self) self._path = path - @cachePerInstance + @cache_per_instance def getSettingDefinition(self, key: str) -> Optional[SettingDefinition]: """Get the SettingDefinition object for a specified key""" @@ -789,7 +789,7 @@ def sendPostponedEmits(self) -> None: signal.emit(signal_arg) @UM.FlameProfiler.profile - @cachePerInstance + @cache_per_instance def hasErrors(self) -> bool: """Check if the container stack has errors""" @@ -812,7 +812,7 @@ def hasErrors(self) -> bool: return False @UM.FlameProfiler.profile - @cachePerInstance + @cache_per_instance_copy_result def getErrorKeys(self) -> List[str]: """Get all the keys that are in an error state in this stack""" diff --git a/UM/Settings/InstanceContainer.py b/UM/Settings/InstanceContainer.py index f1b4fff796..9eee7cb736 100644 --- a/UM/Settings/InstanceContainer.py +++ b/UM/Settings/InstanceContainer.py @@ -10,7 +10,7 @@ from PyQt6.QtCore import QObject, pyqtProperty, pyqtSignal from PyQt6.QtQml import QQmlEngine #To take ownership of this class ourselves. -from UM.Decorators import CachedMemberFunctions, cachePerInstance +from UM.Decorators import CachedMemberFunctions, cache_per_instance, cache_per_instance_copy_result from UM.FastConfigParser import FastConfigParser from UM.Trust import Trust from UM.Decorators import override @@ -308,7 +308,7 @@ def setDirty(self, dirty: bool) -> None: CachedMemberFunctions.clearInstanceCache(self) self._dirty = dirty - @cachePerInstance + @cache_per_instance def getProperty(self, key: str, property_name: str, context: PropertyEvaluationContext = None) -> Any: """:copydoc ContainerInterface::getProperty @@ -327,7 +327,7 @@ def getProperty(self, key: str, property_name: str, context: PropertyEvaluationC return None - @cachePerInstance + @cache_per_instance def hasProperty(self, key: str, property_name: str) -> bool: """:copydoc ContainerInterface::hasProperty @@ -435,7 +435,7 @@ def clear(self) -> None: self.removeInstance(key, postpone_emit=True) self.sendPostponedEmits() - @cachePerInstance + @cache_per_instance_copy_result def getAllKeys(self) -> Set[str]: """Get all the keys of the instances of this container :returns: list of keys @@ -702,7 +702,7 @@ def findInstances(self, **kwargs: Any) -> List[SettingInstance]: return result - @cachePerInstance + @cache_per_instance def getInstance(self, key: str) -> Optional[SettingInstance]: """Get an instance by key""" @@ -776,7 +776,7 @@ def update(self) -> None: self.propertyChanged.emit(key, property_name) self._dirty = True - @cachePerInstance + @cache_per_instance def getDefinition(self) -> DefinitionContainerInterface: """Get the DefinitionContainer used for new instance creation.""" @@ -831,7 +831,7 @@ def sendPostponedEmits(self) -> None: signal, signal_arg = self._postponed_emits.pop(0) signal.emit(*signal_arg) - @cachePerInstance + @cache_per_instance_copy_result def getAllKeysWithUserState(self)-> Set[str]: """Get the keys of all the setting having a User state""" self._instantiateCachedValues() diff --git a/UM/Settings/Models/SettingPropertyProvider.py b/UM/Settings/Models/SettingPropertyProvider.py index 456c0bd849..ef8578b312 100644 --- a/UM/Settings/Models/SettingPropertyProvider.py +++ b/UM/Settings/Models/SettingPropertyProvider.py @@ -7,7 +7,7 @@ from PyQt6.QtQml import QQmlPropertyMap from UM.FlameProfiler import pyqtSlot -from UM.Decorators import CachedMemberFunctions, cachePerInstance +from UM.Decorators import CachedMemberFunctions, cache_per_instance from UM.Logger import Logger from UM.Application import Application from UM.Settings.ContainerStack import ContainerStack @@ -433,7 +433,7 @@ def _updateStackLevels(self) -> None: self._stack_levels = levels self.stackLevelChanged.emit() - @cachePerInstance + @cache_per_instance def _getPropertyValue(self, property_name): # Use the evaluation context to skip certain containers context = PropertyEvaluationContext(self._stack) diff --git a/UM/Settings/SettingDefinition.py b/UM/Settings/SettingDefinition.py index f76c286a70..016f0339b2 100644 --- a/UM/Settings/SettingDefinition.py +++ b/UM/Settings/SettingDefinition.py @@ -9,7 +9,7 @@ import re from typing import Any, List, Dict, Callable, Match, Set, Union, Optional -from UM.Decorators import CachedMemberFunctions, cachePerInstance +from UM.Decorators import CachedMemberFunctions, cache_per_instance, cache_per_instance_copy_result from UM.Logger import Logger from UM.Settings.Interfaces import DefinitionContainerInterface from UM.i18n import i18nCatalog @@ -238,7 +238,7 @@ def relations(self) -> List["SettingRelation"]: return self._relations - @cachePerInstance + @cache_per_instance def relationsAsFrozenSet(self) -> frozenset["SettingRelation"]: """A frozen set of SettingRelation objects of this setting. @@ -255,7 +255,7 @@ def serialize(self) -> str: pass - @cachePerInstance + @cache_per_instance_copy_result def getAllKeys(self) -> Set[str]: """Gets the key of this setting definition and of all its descendants. @@ -301,7 +301,7 @@ def deserialize(self, serialized: Union[str, Dict[str, Any]]) -> None: parsed = json.loads(serialized, object_pairs_hook=collections.OrderedDict) self._deserialize_dict(parsed) - @cachePerInstance + @cache_per_instance def getChild(self, key: str) -> Optional["SettingDefinition"]: """Get a child by key @@ -323,7 +323,7 @@ def getChild(self, key: str) -> Optional["SettingDefinition"]: return None - @cachePerInstance + @cache_per_instance def _matches1l8nProperty(self, property_name: str, value: Any, catalog) -> bool: try: property_value = getattr(self, property_name) @@ -446,7 +446,7 @@ def findDefinitions(self, **kwargs: Any) -> List["SettingDefinition"]: return definitions - @cachePerInstance + @cache_per_instance def isAncestor(self, key: str) -> bool: """Check whether a certain setting is an ancestor of this definition. @@ -460,7 +460,7 @@ def isAncestor(self, key: str) -> bool: return key in self.__ancestors - @cachePerInstance + @cache_per_instance def isDescendant(self, key: str) -> bool: """Check whether a certain setting is a descendant of this definition. @@ -474,7 +474,7 @@ def isDescendant(self, key: str) -> bool: return key in self.__descendants - @cachePerInstance + @cache_per_instance_copy_result def getAncestors(self) -> Set[str]: """Get a set of keys representing the setting's ancestors.""" @@ -737,7 +737,7 @@ def _deserialize_dict(self, serialized: Dict[str, Any]) -> None: self.__ancestors = self._updateAncestors() self.__descendants = self._updateDescendants() - @cachePerInstance + @cache_per_instance_copy_result def _updateAncestors(self) -> Set[str]: result = set() # type: Set[str] diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index 7722b15de3..ec0f9ea062 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -7,7 +7,7 @@ import os from typing import Any, cast, Dict, Iterable, List, Optional, Set, TYPE_CHECKING -from UM.Decorators import CachedMemberFunctions, cachePerInstance +from UM.Decorators import CachedMemberFunctions, cache_per_instance from UM.Settings.Interfaces import ContainerInterface from UM.Signal import Signal, signalemitter from UM.Logger import Logger @@ -92,7 +92,7 @@ def __init__(self, definition: SettingDefinition, container: ContainerInterface, self.__property_values = {} # type: Dict[str, Any] - @cachePerInstance + @cache_per_instance def getPropertyNames(self) -> Iterable[str]: """Get a list of all supported property names""" @@ -136,7 +136,7 @@ def __hash__(self) -> int: def __ne__(self, other: object) -> bool: return not (self == other) - @cachePerInstance + @cache_per_instance def __getattr__(self, name: str) -> Any: if name == "_SettingInstance__property_values": # Prevent infinite recursion when __property_values is not set. diff --git a/tests/TestDecorators.py b/tests/TestDecorators.py index bd67e7fe04..14d17f46ee 100644 --- a/tests/TestDecorators.py +++ b/tests/TestDecorators.py @@ -4,7 +4,7 @@ import pytest -from UM.Decorators import interface, CachedMemberFunctions, cachePerInstance +from UM.Decorators import interface, CachedMemberFunctions, cache_per_instance, cache_per_instance_copy_result def test_interface(): def declare_interface(): @@ -111,11 +111,16 @@ class SomeClass: def __init__(self): self._map = {} - @cachePerInstance + @cache_per_instance def getThing(self, a): bigDeal() return self._map.get(a, None) + @cache_per_instance_copy_result + def getList(self): + bigDeal() + return [234, 456, 789] + def setThing(self, a, b): CachedMemberFunctions.clearInstanceCache(self) self._map[a] = b @@ -145,3 +150,8 @@ def setThing(self, a, b): assert len(CachedMemberFunctions._CachedMemberFunctions__cache) == 2 CachedMemberFunctions.deleteInstanceCache(instance) assert len(CachedMemberFunctions._CachedMemberFunctions__cache) == 1 + + lizt = other.getList() + assert lizt == [234, 456, 789] + lizt.append(111) + assert other.getList() == [234, 456, 789] From 35ec22fc0d75356010901b391284a2744b20b3d0 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 11 Sep 2024 13:08:39 +0200 Subject: [PATCH 19/21] Refactor: Called in both if-branches, so consolidate. part of CURA-11761 --- UM/Settings/SettingInstance.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index ec0f9ea062..dea37894c6 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -161,13 +161,13 @@ def __del__(self) -> None: @call_if_enabled(_traceSetProperty, _isTraceEnabled()) def setProperty(self, name: str, value: Any, container: Optional[ContainerInterface] = None, emit_signals: bool = True) -> None: + CachedMemberFunctions.clearInstanceCache(self) + if SettingDefinition.hasProperty(name): if SettingDefinition.isReadOnlyProperty(name): Logger.log("e", "Tried to set property %s which is a read-only property", name) return - CachedMemberFunctions.clearInstanceCache(self) - if name not in self.__property_values or value != self.__property_values[name]: if isinstance(value, str) and value.strip().startswith("="): value = SettingFunction.SettingFunction(value[1:]) @@ -196,7 +196,6 @@ def setProperty(self, name: str, value: Any, container: Optional[ContainerInterf else: if name == "state": if value == "InstanceState.Calculated": - CachedMemberFunctions.clearInstanceCache(self) if self._state != InstanceState.Calculated: self._state = InstanceState.Calculated if emit_signals: From 33c895873f689362e402f5e4a84e58e52c76baca Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 11 Sep 2024 13:56:15 +0200 Subject: [PATCH 20/21] Also call deletion-handler or parent if added. Previously, if we had added a __del__ method to any parent later, that might not've been called anymore. done as part of CURA-11761 --- UM/Settings/ContainerStack.py | 2 +- UM/Settings/InstanceContainer.py | 2 +- UM/Settings/Models/SettingPropertyProvider.py | 2 +- UM/Settings/SettingDefinition.py | 2 +- UM/Settings/SettingInstance.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/UM/Settings/ContainerStack.py b/UM/Settings/ContainerStack.py index 8547cdbe26..c981ec90f9 100644 --- a/UM/Settings/ContainerStack.py +++ b/UM/Settings/ContainerStack.py @@ -872,8 +872,8 @@ def __repr__(self) -> str: return str(self) def __del__(self) -> None: - # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. CachedMemberFunctions.deleteInstanceCache(self) + getattr(super(), "__del__", lambda s: None)(self) _containerRegistry = ContainerRegistryInterface() # type: ContainerRegistryInterface diff --git a/UM/Settings/InstanceContainer.py b/UM/Settings/InstanceContainer.py index 9eee7cb736..5eba4ddebc 100644 --- a/UM/Settings/InstanceContainer.py +++ b/UM/Settings/InstanceContainer.py @@ -817,8 +817,8 @@ def __repr__(self) -> str: return str(self) def __del__(self) -> None: - # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. CachedMemberFunctions.deleteInstanceCache(self) + getattr(super(), "__del__", lambda s: None)(self) def sendPostponedEmits(self) -> None: """Send the postponed emits diff --git a/UM/Settings/Models/SettingPropertyProvider.py b/UM/Settings/Models/SettingPropertyProvider.py index ef8578b312..700fa46260 100644 --- a/UM/Settings/Models/SettingPropertyProvider.py +++ b/UM/Settings/Models/SettingPropertyProvider.py @@ -472,5 +472,5 @@ def _getPropertyValue(self, property_name): return str(property_value) def __del__(self) -> None: - # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. CachedMemberFunctions.deleteInstanceCache(self) + getattr(super(), "__del__", lambda s: None)(self) diff --git a/UM/Settings/SettingDefinition.py b/UM/Settings/SettingDefinition.py index 016f0339b2..827646c807 100644 --- a/UM/Settings/SettingDefinition.py +++ b/UM/Settings/SettingDefinition.py @@ -190,8 +190,8 @@ def __setstate__(self, state): self._all_keys = set() def __del__(self) -> None: - # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. CachedMemberFunctions.deleteInstanceCache(self) + getattr(super(), "__del__", lambda s: None)(self) @property def key(self) -> str: diff --git a/UM/Settings/SettingInstance.py b/UM/Settings/SettingInstance.py index dea37894c6..8c30aadc4c 100644 --- a/UM/Settings/SettingInstance.py +++ b/UM/Settings/SettingInstance.py @@ -156,8 +156,8 @@ def __getattr__(self, name: str) -> Any: raise AttributeError("'SettingInstance' object has no attribute '{0}'".format(name)) def __del__(self) -> None: - # None of the 'parents' seem to have __del__, so OK not to call `super.del` here. CachedMemberFunctions.deleteInstanceCache(self) + getattr(super(), "__del__", lambda s: None)(self) @call_if_enabled(_traceSetProperty, _isTraceEnabled()) def setProperty(self, name: str, value: Any, container: Optional[ContainerInterface] = None, emit_signals: bool = True) -> None: From ddd53ff9478b73f0d2282b657375508cbe3c0501 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 18 Sep 2024 08:17:53 +0200 Subject: [PATCH 21/21] Add thread-lock to cache to prevent race-condition(s). part of CURA-11761 --- UM/Decorators.py | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/UM/Decorators.py b/UM/Decorators.py index 344513faa9..94c0e1e32b 100644 --- a/UM/Decorators.py +++ b/UM/Decorators.py @@ -5,6 +5,7 @@ from functools import lru_cache, partial import warnings import inspect +from threading import Lock from UM.Logger import Logger from typing import Callable, Any @@ -145,17 +146,26 @@ class CachedMemberFunctions: """Helper class to handle instance-cache w.r.t. results of member-functions decorated with '@cache_per_instance'.""" __cache = {} + __locks = {} + + @classmethod + def _getInstanceLock(cls, instance): + if instance not in cls.__locks: + cls.__locks[instance] = Lock() + return cls.__locks[instance] @classmethod def clearInstanceCache(cls, instance): """Clear all the cache-entries for the specified instance.""" - cls.__cache[instance] = {} + with cls._getInstanceLock(instance): + cls.__cache[instance] = {} @classmethod def deleteInstanceCache(cls, instance): """Completely delete the entry of the specified instance.""" - if instance in cls.__cache: - del cls.__cache[instance] + with cls._getInstanceLock(instance): + if instance in cls.__cache: + del cls.__cache[instance] @classmethod def callMemberFunction(cls, instance, function, *args, **kwargs): @@ -164,11 +174,14 @@ def callMemberFunction(cls, instance, function, *args, **kwargs): # NOTE The `lru_cache` can't handle keyword-arguments (because it's a dict). # We could make a frozendict, but that's probably a lot more hassle than it's worth, so just call normally. return function(instance, *args, **kwargs) - if instance not in cls.__cache: - cls.__cache[instance] = {} - if function not in cls.__cache[instance]: - cls.__cache[instance][function] = lru_cache()(partial(function, instance)) - return cls.__cache[instance][function](*args) + with cls._getInstanceLock(instance): + if instance not in cls.__cache: + cls.__cache[instance] = {} + if function not in cls.__cache[instance]: + cls.__cache[instance][function] = lru_cache()(partial(function, instance)) + func = cls.__cache[instance][function] + # Need to call the function outside the locked part, as it may introduce a race-condition otherwise. + return func(*args) def cache_per_instance(function):